Bug 144905

Created attachment 252962 [details] <WIP>

Created attachment 253013 [details] Patch

Created attachment 253015 [details] Patch

Created attachment 253016 [details] Patch

Comment on attachment 253016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253016&action=review > Source/WebCore/css/CSSPrimitiveValue.h:394 > + CSSPrimitiveValue::CSSPrimitiveValue(DashboardRegion*) Need to remove the CSSPrimitiveValue:: prefix here.

(In reply to comment #5) > Comment on attachment 253016 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253016&action=review > > > Source/WebCore/css/CSSPrimitiveValue.h:394 > > + CSSPrimitiveValue::CSSPrimitiveValue(DashboardRegion*) > > Need to remove the CSSPrimitiveValue:: prefix here. Oh, I missed it. Thank you for your comment :)

Created attachment 253019 [details] Patch

Created attachment 253075 [details] <WIP>

Created attachment 253090 [details] Patch

Created attachment 253092 [details] Patch

Comment on attachment 253092 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253092&action=review review- because the CSSPrimitiveValue part seems wrong; otherwise all looks fine > Source/WebCore/css/BasicShapeFunctions.cpp:52 > +static RefPtr<CSSPrimitiveValue> basicShapeRadiusToCSSValue(const RenderStyle* style, CSSValuePool& pool, const BasicShapeRadius& radius) Seems like this should return Ref. The failure case that currently returns null could and should be changed to return another value. Callers can’t handle null! Also seems that the style argument should be changed to a reference rather than a pointer. > Source/WebCore/css/CSSPrimitiveValue.h:42 > +#if ENABLE(DASHBOARD_SUPPORT) > class DashboardRegion; > +#endif Please don’t do this. It’s OK to forward declare this even if it’s not used. If you did want to make it conditional we’d move it to a separate paragraph. > Source/WebCore/css/CSSPrimitiveValue.h:393 > + init(Ref<T>(*val)); What guarantees the pointer is non-null here? We can’t just do a * on a pointer that might be null. > Source/WebCore/css/CSSPrimitiveValue.h:400 > + CSSPrimitiveValue(CSSBasicShape*); > + CSSPrimitiveValue(CSSCalcValue*); > +#if ENABLE(DASHBOARD_SUPPORT) > + CSSPrimitiveValue(DashboardRegion*); > +#endif Why are these needed? I think this is a bad idea. > Source/WebCore/css/CSSPrimitiveValue.h:428 > + void init(RefPtr<DashboardRegion>&&); // FIXME: Dashboard region should not be a primitive value. Why RefPtr instead of Ref? Do callers really need to do this with null? > Source/WebCore/css/CSSPrimitiveValue.h:431 > + void init(RefPtr<CSSBasicShape>&&); > + void init(RefPtr<CSSCalcValue>&&); Why RefPtr instead of Ref? Do callers really need to do this with null? > Source/WebCore/css/CSSPrimitiveValue.h:457 > +#if ENABLE(DASHBOARD_SUPPORT) > DashboardRegion* region; > +#endif I don’t think this is an improvement. Lets go a little lighter on the #if here, unless you are sure we should do it. I think it does no harm to compile this on platforms that don’t support it. > Source/WebCore/css/FontLoader.cpp:53 > + return adoptRef<LoadFontCallback>(*new LoadFontCallback(numLoading, fontLoader, loadCallback, errorCallback)); I don’t understand the use of adoptRef<LoadFontCallback> here. Should just be adoptRef with no <> after it.

Created attachment 253822 [details] <WIP> only first and last comment have been addressed in this patch

(In reply to comment #11) > Comment on attachment 253092 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253092&action=review > > review- because the CSSPrimitiveValue part seems wrong; otherwise all looks > fine > > > Source/WebCore/css/BasicShapeFunctions.cpp:52 > > +static RefPtr<CSSPrimitiveValue> basicShapeRadiusToCSSValue(const RenderStyle* style, CSSValuePool& pool, const BasicShapeRadius& radius) > > Seems like this should return Ref. The failure case that currently returns > null could and should be changed to return another value. Callers can’t > handle null! Also seems that the style argument should be changed to a > reference rather than a pointer. > I changed this to Ref. And let default return be pool.createIdentifierValue(CSSValueClosestSide). I'm not sure this is right decision. > > Source/WebCore/css/CSSPrimitiveValue.h:42 > > +#if ENABLE(DASHBOARD_SUPPORT) > > class DashboardRegion; > > +#endif > > Please don’t do this. It’s OK to forward declare this even if it’s not used. > If you did want to make it conditional we’d move it to a separate paragraph. > I removed #if directive as you said because it is not necessary. > > Source/WebCore/css/CSSPrimitiveValue.h:393 > > + init(Ref<T>(*val)); > > What guarantees the pointer is non-null here? We can’t just do a * on a > pointer that might be null. > About this, I don't have a good idea. I just added a null check to this, but it doesn't seem right. Do you have any advice? > > Source/WebCore/css/CSSPrimitiveValue.h:400 > > + CSSPrimitiveValue(CSSBasicShape*); > > + CSSPrimitiveValue(CSSCalcValue*); > > +#if ENABLE(DASHBOARD_SUPPORT) > > + CSSPrimitiveValue(DashboardRegion*); > > +#endif > > Why are these needed? I think this is a bad idea. > I removed other things except CSSPrimitveValue(CSSCalcValue*), because without this one, 'error: call of overloaded ‘init(WTF::Ref<WebCore::CSSCalcValue>)’ is ambiguous' occurs. > > Source/WebCore/css/CSSPrimitiveValue.h:428 > > + void init(RefPtr<DashboardRegion>&&); // FIXME: Dashboard region should not be a primitive value. > > Why RefPtr instead of Ref? Do callers really need to do this with null? > I changed this one with Ref according to your comment. > > Source/WebCore/css/CSSPrimitiveValue.h:431 > > + void init(RefPtr<CSSBasicShape>&&); > > + void init(RefPtr<CSSCalcValue>&&); > > Why RefPtr instead of Ref? Do callers really need to do this with null? > I changed this one with Ref according to your comment. > > Source/WebCore/css/CSSPrimitiveValue.h:457 > > +#if ENABLE(DASHBOARD_SUPPORT) > > DashboardRegion* region; > > +#endif > > I don’t think this is an improvement. Lets go a little lighter on the #if > here, unless you are sure we should do it. I think it does no harm to > compile this on platforms that don’t support it. > As you said, I removed this unnecessary guard. > > Source/WebCore/css/FontLoader.cpp:53 > > + return adoptRef<LoadFontCallback>(*new LoadFontCallback(numLoading, fontLoader, loadCallback, errorCallback)); > > I don’t understand the use of adoptRef<LoadFontCallback> here. Should just > be adoptRef with no <> after it. I revised this one.

Created attachment 253839 [details] Patch

Created attachment 253843 [details] Patch

Comment on attachment 253843 [details] Patch Attachment 253843 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6173793483489280 New failing tests: fast/css/large-value-csstext.html

Created attachment 253844 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5

Comment on attachment 253843 [details] Patch Attachment 253843 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6116746251468800 New failing tests: fast/css/large-value-csstext.html

Created attachment 253845 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Comment on attachment 253843 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253843&action=review > Source/WebCore/css/BasicShapeFunctions.cpp:63 > - ASSERT_NOT_REACHED(); > - return 0; > + return pool.createIdentifierValue(CSSValueClosestSide); Should not delete the ASSERT_NOT_REACHED. > Source/WebCore/css/BasicShapeFunctions.cpp:74 > - RefPtr<CSSBasicShapeCircle> circleValue = CSSBasicShapeCircle::create(); > + Ref<CSSBasicShapeCircle> circleValue = CSSBasicShapeCircle::create(); Maybe auto instead of writing out the type? > Source/WebCore/css/BasicShapeFunctions.cpp:84 > - RefPtr<CSSBasicShapeEllipse> ellipseValue = CSSBasicShapeEllipse::create(); > + Ref<CSSBasicShapeEllipse> ellipseValue = CSSBasicShapeEllipse::create(); Maybe auto instead of writing out the type? > Source/WebCore/css/BasicShapeFunctions.cpp:95 > - RefPtr<CSSBasicShapePolygon> polygonValue = CSSBasicShapePolygon::create(); > + Ref<CSSBasicShapePolygon> polygonValue = CSSBasicShapePolygon::create(); Maybe auto instead of writing out the type? > Source/WebCore/css/BasicShapeFunctions.cpp:126 > + return pool.createValue(basicShapeValue.releaseNonNull()); This is wrong. If we hit the default case above then basicShapeValue is null. Really need to fix that. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1956 > + return fillSizeToCSSValue(layers->size(), *style.get()); No need for the ".get()" here. Same for all the other sites below! > Source/WebCore/css/CSSParser.cpp:5604 > Blank line here seems out of place (between declaration and initialization of lineNames). > Source/WebCore/css/CSSParser.cpp:5605 > + if (previousNamedAreaTrailingLineNames != nullptr) WebKit coding style says you just do: if (previousNamedAreaTrailingLineNames) > Source/WebCore/css/CSSPrimitiveValue.cpp:240 > +CSSPrimitiveValue::CSSPrimitiveValue(CSSCalcValue* val) Please use the name "value", not "val". > Source/WebCore/css/CSSPrimitiveValue.cpp:316 > + RefPtr<CSSCalcValue> calcValue = CSSCalcValue::create(length.calculationValue(), style); > + init(WTF::move(calcValue)); This would read better without the local variable and you could remove the braces too. > Source/WebCore/css/CSSPrimitiveValue.h:392 > + if (!val) > + init(Ref<T>(*val)); This looks like it will break everything; I bet it’s why tests are failing! I think you mean if (val). But also is this behavior OK for null pointers. > Source/WebCore/css/CSSPrimitiveValue.h:395 > + CSSPrimitiveValue(CSSCalcValue*); Pretty messy. Maybe we can change this at the call site to use Ref or RefPtr rather than a raw pointer.

(In reply to comment #20) > Comment on attachment 253843 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253843&action=review > > > Source/WebCore/css/BasicShapeFunctions.cpp:63 > > - ASSERT_NOT_REACHED(); > > - return 0; > > + return pool.createIdentifierValue(CSSValueClosestSide); > > Should not delete the ASSERT_NOT_REACHED. > I readded ASSERT_NOT_REACHED here. > > Source/WebCore/css/BasicShapeFunctions.cpp:74 > > - RefPtr<CSSBasicShapeCircle> circleValue = CSSBasicShapeCircle::create(); > > + Ref<CSSBasicShapeCircle> circleValue = CSSBasicShapeCircle::create(); > > Maybe auto instead of writing out the type? > I changed the type as auto instead. > > Source/WebCore/css/BasicShapeFunctions.cpp:84 > > - RefPtr<CSSBasicShapeEllipse> ellipseValue = CSSBasicShapeEllipse::create(); > > + Ref<CSSBasicShapeEllipse> ellipseValue = CSSBasicShapeEllipse::create(); > > Maybe auto instead of writing out the type? > ditto. > > Source/WebCore/css/BasicShapeFunctions.cpp:95 > > - RefPtr<CSSBasicShapePolygon> polygonValue = CSSBasicShapePolygon::create(); > > + Ref<CSSBasicShapePolygon> polygonValue = CSSBasicShapePolygon::create(); > > Maybe auto instead of writing out the type? > ditto. > > Source/WebCore/css/BasicShapeFunctions.cpp:126 > > + return pool.createValue(basicShapeValue.releaseNonNull()); > > This is wrong. If we hit the default case above then basicShapeValue is > null. Really need to fix that. > It seems that Ref<BasicShape> basicShapeForValue(); function is implemented as same as this one because CSSBasicShape is abstract class and type() is pure virtual. This is the same for BasicShape class too. For readability it could be an option to add ASSERT_NOT_REACHED in default in both basicShapeForValue() and valueForBasicShape(). But by releaseNonNull(), null check assert is already being done, so it doesn't seems to be necessary. > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1956 > > + return fillSizeToCSSValue(layers->size(), *style.get()); > > No need for the ".get()" here. Same for all the other sites below! > Ah, truly it is :) Thank you for your pointing out. > > Source/WebCore/css/CSSParser.cpp:5604 > > > > Blank line here seems out of place (between declaration and initialization > of lineNames). > I removed this unnecessary blank line. > > Source/WebCore/css/CSSParser.cpp:5605 > > + if (previousNamedAreaTrailingLineNames != nullptr) > > WebKit coding style says you just do: > > if (previousNamedAreaTrailingLineNames) > I revised this one as you commented. > > Source/WebCore/css/CSSPrimitiveValue.cpp:240 > > +CSSPrimitiveValue::CSSPrimitiveValue(CSSCalcValue* val) > > Please use the name "value", not "val". > I removed ctor CSSPrimitiveValue(T* value) for now, because it is being used no more, and I was not sure just adding if (val) is right behavior, as you commented also. > > Source/WebCore/css/CSSPrimitiveValue.cpp:316 > > + RefPtr<CSSCalcValue> calcValue = CSSCalcValue::create(length.calculationValue(), style); > > + init(WTF::move(calcValue)); > > This would read better without the local variable and you could remove the > braces too. > I revised this one as you commented. > > Source/WebCore/css/CSSPrimitiveValue.h:392 > > + if (!val) > > + init(Ref<T>(*val)); > > This looks like it will break everything; I bet it’s why tests are failing! > I think you mean if (val). But also is this behavior OK for null pointers. > For now I removed this ctor as I wrote above. > > Source/WebCore/css/CSSPrimitiveValue.h:395 > > + CSSPrimitiveValue(CSSCalcValue*); > > Pretty messy. Maybe we can change this at the call site to use Ref or RefPtr > rather than a raw pointer. I revised the callsites to use Ref or RefPtr instead of raw pointers.

Created attachment 253971 [details] Patch

Comment on attachment 253971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253971&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2744 > - return cssValuePool().createValue(firstRegion.release()); > + return cssValuePool().createValue(firstRegion.releaseNonNull()); I don’t see a guarantee here that there is a firstRegion. Can’t count be zero? I’m talking about a runtime problem here, not an assertion. Naturally releaseNonNull will assert, but I am talking about in production without assertions present to inform us. > Source/WebCore/css/CSSPrimitiveValue.h:418 > + void init(RefPtr<CSSCalcValue>&&); Need to explain why this one is a RefPtr instead of a Ref.

(In reply to comment #23) > Comment on attachment 253971 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253971&action=review > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2744 > > - return cssValuePool().createValue(firstRegion.release()); > > + return cssValuePool().createValue(firstRegion.releaseNonNull()); > > I don’t see a guarantee here that there is a firstRegion. Can’t count be > zero? > As you pointing out, it seems that if getPropertyValue is called when dashboardRegion was not being set, count can be zero. So I revised init parameter type to RefPtr<DashboardRegion>&&. > I’m talking about a runtime problem here, not an assertion. Naturally > releaseNonNull will assert, but I am talking about in production without > assertions present to inform us. > > > Source/WebCore/css/CSSPrimitiveValue.h:418 > > + void init(RefPtr<CSSCalcValue>&&); > > Need to explain why this one is a RefPtr instead of a Ref. In CSSCalculationValue.h, the two CSSCalcValue::create()'s return type is RefPtr because they can return nullptr, and init(CSSCalcValue::create(length.calculationValue(), style)); is called in CSSPrimitiveValue::CSSPrimitiveValue(const Length&, const RenderStyle&) for example. So I used RefPtr for CSSCalcValue.

Created attachment 254104 [details] Patch

Comment on attachment 254104 [details] Patch Clearing flags on attachment: 254104 Committed r185238: <http://trac.webkit.org/changeset/185238>

All reviewed patches have been landed. Closing bug.