Closed Bug 1312155 Opened 3 years ago Closed 3 years ago

Move border-width constants out of nsPresContext

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: manishearth, Assigned: vi.le, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [lang=c++])

Attachments

(1 file, 4 obsolete files)

nsPresContext has a table `mBorderWidthTable` (https://dxr.mozilla.org/mozilla-central/rev/5639a9f476d08f300c079117e61697f5026b6367/layout/base/nsPresContext.h#1315), which contains the mapping of the "thin" "medium" and "thick" keywords to pixel values for border widths (https://dxr.mozilla.org/mozilla-central/rev/5639a9f476d08f300c079117e61697f5026b6367/layout/base/nsPresContext.cpp#885)

We should remove this member and the GetBorderWidthTable method and instead make this into a regular function somewhere which uses a static table. (and replace the call sites of GetBorderWidthTable https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3AnsPresContext%3A%3AGetBorderWidthTable%28%29 with this new function)
Hello,

can I work on this please ?
Go ahead!

Feel free to ask any questions you have!
Ok, thanks
Hello, what's the best way to ensure that I haven't broken something?

Thanks
Ensure it builds, upload the patch, and ask us to push to try for you.
Attachment #8805642 - Flags: review?(manishearth)
Assignee: nobody → sky
Just submitted the patch. I'm not sure about where I should put the function.
Comment on attachment 8805642 [details] [diff] [review]
move-border-width-constants.patch

Review of attachment 8805642 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is okay, but let's see what Cameron says.

::: layout/base/nsPresContext.h
@@ +880,5 @@
>    uint64_t FramesReflowedCount() {
>      return mFramesReflowed;
>    }
>  
> +  nscoord GetBorderWidthTable(int borderWidth)

`GetBorderWidthForKeyword()` perhaps?
Attachment #8805642 - Flags: review?(manishearth)
Attachment #8805642 - Flags: review?(cam)
Attachment #8805642 - Flags: feedback+
Comment on attachment 8805642 [details] [diff] [review]
move-border-width-constants.patch

Review of attachment 8805642 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks very much for the patch, Vincent!  A few comments below on things to tweak.

::: layout/base/nsPresContext.h
@@ +880,5 @@
>    uint64_t FramesReflowedCount() {
>      return mFramesReflowed;
>    }
>  
> +  nscoord GetBorderWidthTable(int borderWidth)

"GetBorderWidthForKeyword" sounds good to me.

Nit: let's name the argument "aBorderWidthKeyword".  I think this makes it a little clearer that the value (since it's an int) is not a width itself.  Also, the initial "a" is the standard Mozilla style for arguments:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Prefixes

Also, please make this a static method.  Then, please update the call sites to be nsPresContext::GetBorderWidthForKeyword rather than calling the method on the nsPresContext instance.

@@ +882,5 @@
>    }
>  
> +  nscoord GetBorderWidthTable(int borderWidth)
> +  {
> +      // This table maps border-width enums 'thin', 'medium', 'thick'

Please use two-space indenting.

@@ +884,5 @@
> +  nscoord GetBorderWidthTable(int borderWidth)
> +  {
> +      // This table maps border-width enums 'thin', 'medium', 'thick'
> +      // to actual nscoord values.
> +      static nscoord nscoord[] = {CSSPixelsToAppUnits(1), CSSPixelsToAppUnits(3), CSSPixelsToAppUnits(5)};

Please make this const and name the variable differently from the type.  Maybe "kBorderWidths"?

Let's assert that the argument is a valid border width value so that we can catch any reads off the end of the array.

  MOZ_ASSERT(aBorderWidthKeyword < mozilla::ArrayLength(kBorderWidths));
Attachment #8805642 - Flags: review?(cam)
Attachment #8805642 - Attachment is obsolete: true
Attachment #8806426 - Flags: review?(cam)
Thank you for the review!
Comment on attachment 8806426 [details] [diff] [review]
move-border-width-constants.patch

Review of attachment 8806426 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thanks!  Two more very minor things.  r=me with that.

::: layout/base/nsPresContext.h
@@ +885,5 @@
> +  {
> +    MOZ_ASSERT(aBorderWidthKeyword < mozilla::ArrayLength(kBorderWidths));
> +    // This table maps border-width enums 'thin', 'medium', 'thick'
> +    // to actual nscoord values.
> +    static const nscoord kBorderWidths[] = {CSSPixelsToAppUnits(1), CSSPixelsToAppUnits(3), CSSPixelsToAppUnits(5)};

Nit: sorry, missed this first time around.  Please keep to 80 character line widths.  (Although I know that we violate that in places.)

You know, it might be more concise to factor out these CSSPixelsToAppUnits() and just call it once in the return statement below.

::: layout/style/nsRuleNode.cpp
@@ +7491,5 @@
>                       value.GetIntValue() == NS_STYLE_BORDER_WIDTH_MEDIUM ||
>                       value.GetIntValue() == NS_STYLE_BORDER_WIDTH_THICK,
>                       "Unexpected enum value");
>          border->SetBorderWidth(side,
> +                               (nsPresContext::GetBorderWidthForKeyword(value.GetIntValue())));

Nit: I don't think we need this extra pair of parens around this second argument to SetBorderWidth.  Similarly for the other three GetBorderWidthForKeyword calls below.
Attachment #8806426 - Flags: review?(cam) → review+
"You know, it might be more concise to factor out these CSSPixelsToAppUnits() and just call it once in the return statement below"

What do you mean? Do you mean to map NS_STYLE_BORDER_WIDTH_* to the corresponding argument to CSSPixelsToAppUnits() i.e 1, 3 and 5 ?

Thank you for for taking the time to help newcomers :-)
Attachment #8806426 - Attachment is obsolete: true
Attachment #8808957 - Flags: review?(cam)
(In reply to sky from comment #13)
> "You know, it might be more concise to factor out these
> CSSPixelsToAppUnits() and just call it once in the return statement below"
> 
> What do you mean? Do you mean to map NS_STYLE_BORDER_WIDTH_* to the
> corresponding argument to CSSPixelsToAppUnits() i.e 1, 3 and 5 ?

I'm sorry, I missed this comment earlier.  I meant:

  return CSSPixelsToAppUnits(kBorderWidths[aBorderWidthKeyword]);

and then just have the 1, 3, and 5 in the array.  But I think it looks clearer in your current patch, so I think that's fine.

> Thank you for for taking the time to help newcomers :-)

And thanks for the patch. :-)
Attachment #8808957 - Flags: review?(cam) → review+
I pushed a try run for you.  Results will appear here:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a10d06e073e3d0229c81e12792c41da8a787e856

If everything looks good once it's done (there may be some intermittent, unrelated test failures) please set the checkin-needed keyword on this bug and the sheriffs will land the patch for you.
> /builds/slave/try-lx-d-000000000000000000000/build/src/obj-firefox/dist/include
> /nsPresContext.h:886:116: error: 'kBorderWidths' was not declared in this scope

This comes from the assertion being at the wrong place, right?
Yes, indeed it does.  Just move it down below the array.
Attachment #8808957 - Attachment is obsolete: true
They are still some errors :

/home/worker/workspace/build/src/obj-firefox/dist/include/nsPresContext.h:893:261: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]

does this come from the argument to GetBorderWidthForKeyword being int and not unsigned int?

When building firefox, how can I isolate errors & warnings from a given file ?
Yeah, unfortunately the build produces very many warnings.  On the build machines, certain directories have their warnings turned into errors.  You can enable this for your own build by adding

  ac_add_options --enable-warnings-as-errors

to your .mozconfig file.  If you are using a different compiler from the build machines, you might get errors that won't turn up on the build machines.

As for this specific problem, yes, it's because the argument is signed, and the ArrayLength() function return value is unsigned.  This is an indication that we should probably assert that the argument is >= 0, too!  And then for the existing assertion expression, you can just convert the left hand side to size_t (which is what ArrayLength returns), so something like |size_t(aBorderWidthKeyword) < ArrayLength(...)|.
BTW, we have a similar border width table in [1]. Do we want to convert that as well?

[1] http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/layout/base/StaticPresData.cpp#42-44
Yes, but once this bug has landed, we can just call StaticPresData::GetBorderWidthForKeyword() call through to nsPresContext::GetBorderWidthForKeyword(), rather than have its own copy of the method.  Or we can update the current (two) callers in nsStyleStruct.cpp to just call nsPresContext::GetBorderWidthForKeyword() directly, and then remove GetBorderWidthForKeyword from StaticPresData.
Attachment #8809305 - Attachment is obsolete: true
Attachment #8811383 - Flags: review?(cam)
Attachment #8811383 - Flags: review?(cam) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8294478fc1bb
Move border-width constants out of nsPresContext; r=heycam
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8294478fc1bb
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.