Incorrect call to StyledElement::setInlineStyleProperty in ImageDocument::createDocumentStructure

[Michael Catanzaro]

Here is another good GCC 7 warning:

[3172/5861] Building CXX object Source...s/WebCore.dir/html/ImageDocument.cpp.o
../../Source/WebCore/html/ImageDocument.cpp: In member function ‘void WebCore::ImageDocument::createDocumentStructure()’:
../../Source/WebCore/html/ImageDocument.cpp:226:103: warning: enum constant in boolean context [-Wint-in-bool-context]
         body->setInlineStyleProperty(CSSPropertyBackgroundColor, "white", CSSPrimitiveValue::CSS_IDENT);

This call to StyledElement::setInlineStyleProperty inside ImageDocument::createDocumentStructure is incorrect:

    if (MIMETypeRegistry::isPDFMIMEType(document().loader()->responseMIMEType()))
        body->setInlineStyleProperty(CSSPropertyBackgroundColor, "white", CSSPrimitiveValue::CSS_IDENT);

Here are the overloads:

    bool setInlineStyleProperty(CSSPropertyID, CSSValueID identifier, bool important = false);
    bool setInlineStyleProperty(CSSPropertyID, CSSPropertyID identifier, bool important = false);
    WEBCORE_EXPORT bool setInlineStyleProperty(CSSPropertyID, double value, CSSPrimitiveValue::UnitType, bool important = false);
    WEBCORE_EXPORT bool setInlineStyleProperty(CSSPropertyID, const String& value, bool important = false);

Currently it is calling the final overload (CSSPropertyID, const String& value, bool important = false). Surely it is not intended for CSSPrimitiveValue::CSS_IDENT to be implicitly converted to bool (important = true), as is currently occurring. I think the proper fix is probably to just remove that argument. However, I am not sure, because I'm not sure how the important flag is supposed to be used. This is a behavior change, so it would be good for someone familiar with how the code is intended to work to review it.

[Michael Catanzaro]

Created attachment 315362 [details]
Patch

[Michael Catanzaro]

(The way to do this with no behavior change would be to replace CSSPrimitiveValue::CSS_IDENT with true. But I suspect that's not right.)

[Darin Adler]

Comment on attachment 315362 [details]
Patch

There was no great harm in the background color property being set as important, but it was definitely not intentional either.

[Build Bot]

Comment on attachment 315362 [details]
Patch

Attachment 315362 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4115353

New failing tests:
imported/w3c/web-platform-tests/IndexedDB/large-nested-cloning.html

[Build Bot]

Created attachment 315380 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Michael Catanzaro]

(In reply to Darin Adler from comment #3)
> There was no great harm in the background color property being set as
> important, but it was definitely not intentional either.

Is there any documentation for what property importance means? I'm curious.

[Michael Catanzaro]

Committed r219479: <http://trac.webkit.org/changeset/219479>

[Tim Horton]

(In reply to Michael Catanzaro from comment #6)
> (In reply to Darin Adler from comment #3)
> > There was no great harm in the background color property being set as
> > important, but it was definitely not intentional either.
> 
> Is there any documentation for what property importance means? I'm curious.

Pretty sure it's the "!important" bit in CSS, which is ... well documented elsewhere (see MDN maybe?).

[Michael Catanzaro]

OK, thanks!