[webkit-reviews] review denied: [Bug 121415] The size of the background colour is not properly adjusted when the font size is being changed : [Attachment 218389] New solution for the font size change and backgroundcolor issue

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Artur Moryc <a.moryc at samsung.com>'s
request for review:
Bug 121415: The size of the background colour is not properly adjusted when the
font size is being changed
https://bugs.webkit.org/show_bug.cgi?id=121415

Attachment 218389: New solution for the font size change and backgroundcolor
issue
https://bugs.webkit.org/attachment.cgi?id=218389&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=218389&action=review


>
LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment-expec
ted.txt:3
> +| <span>
> +|   style="background-color: rgb(255, 0, 0);"

We shouldn't have this outer span with background color.

>
LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment-if-co
ntinue-style.html:12
> +<script src=../editing.js language="JavaScript" type="text/JavaScript"
></script>

Why does this script element have both language and type specified? Neither
attribute is needed here.
Also, we need to wrap the path in quotation marks.

>
LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment-if-co
ntinue-style.html:18
> +    function changeFontSizeForOneBackgroundcolorTest() {

Nit: the function shouldn't be indented.

>
LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment-if-co
ntinue-style.html:23
> +	   for(fontSize = maxFontSize-1; fontSize >= minFontSize; fontSize--) {


Nit: Space around -.

>
LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment.html:
5
> +<script src=../editing.js language="JavaScript" type="text/JavaScript"
></script>

Ditto.

>
LayoutTests/editing/inserting/fontsize-varying-backgroundcolor-adjustment.html:
11
> +    function changeFontSizeForOneBackgroundcolorTest() {

Ditto.

>
LayoutTests/editing/inserting/fontsize-varying-cssBackgroundcolor-adjustment-if
-continue-style.html:17
> +<script src=../editing.js language="JavaScript" type="text/JavaScript"
></script>

Ditto.

>
LayoutTests/editing/inserting/fontsize-varying-cssBackgroundcolor-adjustment-if
-continue-style.html:24
> +	   var minFontSize = 1;

Nit: Wrong indentation. Each indentation must be done by 4 spaces.

> Source/WebCore/ChangeLog:26
> +	   * editing/ApplyStyleCommand.cpp:
> +	   (WebCore::ApplyStyleCommand::applyInlineStyleChange):
> +	   * editing/EditingStyle.cpp:
> +	   (WebCore::ColorValueToRGBA):
> +	   (WebCore::rgbaBackgroundColorInEffect):
> +	   (WebCore::EditingStyle::init):
> +	   (WebCore::EditingStyle::styleAtSelectionStart):
> +	   (WebCore::EditingStyle::backgroundColorInEffect):
> +	   * editing/EditingStyle.h:

You need to have per-function level comments.

> Source/WebCore/editing/EditingStyle.cpp:390
> +static RGBA32 colorValueToRGBA(Color colorValue)
> +{
> +    if (!colorValue.isValid() || !colorValue.alpha())
> +	   return Color::transparent;

Why is invalid color treated as transparent?
It seems like this is specific to background color and better be delt with in
rgbaBackgroundColorInEffect itself.

> Source/WebCore/editing/EditingStyle.cpp:1494
> +	   color = style->visitedDependentColor(CSSPropertyBackgroundColor);

We should definitely not use visitedDependentColor.
r- because this leaks history information.

> Source/WebCore/editing/EditingStyle.cpp:-1608
> -PassRefPtr<CSSValue> backgroundColorInEffect(Node* node)
> -{

Why are you rewriting this function? This was a perfectly reasonable
implementation.