Closed Bug 1295084 Opened 8 years ago Closed 8 years ago

Move nsStyleImageLayers::Position to a different scope

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: u459114, Assigned: TYLin)

Details

Attachments

(4 files)

Bug 1295084 Part 1 - Move FragmentOrURL into mozilla namespace.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1295084 Part 2 - Move two Position methods into nsStyleImageLayers.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1295084 Part 3 - Move nsStyleImageLayers::Position to mozilla::Position.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1295084 Part 4 - Rename Position::PositionCoord to Position::Coord.
58 bytes, text/x-review-board-request
dholbert
: review+
Details 
http://searchfox.org/mozilla-central/source/layout/style/nsStyleStruct.h#1740

dholbert's comment
  // XXXdholbert nsStyleImageLayers::Position should probably be moved to a
  // different scope, since we're now using it in multiple style structs.
  typedef nsStyleImageLayers::Position Position;
QA Contact: hshih
Status: NEW → ASSIGNED
Assignee: cku → hshih
QA Contact: hshih
Jerry, I have patches ready for this bug.
Assignee: hshih → tlin
Comment on attachment 8784760 [details]
Bug 1295084 Part 1 - Move FragmentOrURL into mozilla namespace.

https://reviewboard.mozilla.org/r/74078/#review72036

r=me -- just one whitespace nit:

(Side observation: This patch leaves us with two "namespace mozilla{...}" blocks in nsStyleStruct.h, which feels kinda awkward. I was going to suggest merging them, but that looks like it'd involve moving classes further away from where they're used, purely for stylistic purposes, which would also be bad; and in the long run this file will all eventually be in one giant namespace mozilla{...} anyway. So, this is fine as-is.)

::: layout/style/nsStyleStruct.h:696
(Diff revision 1)
>  
>    struct Layer;
>    friend struct Layer;
>    struct Layer {
>      nsStyleImage  mImage;         // [reset]
> -    FragmentOrURL mSourceURI;     // [reset]
> +    mozilla::FragmentOrURL mSourceURI;     // [reset]

Please reduce the whitespace before "// [reset]" on this line down to 2 spaces, now that it's not serving a useful alignment purpose anymore.
Attachment #8784760 - Flags: review?(dholbert) → review+
Comment on attachment 8784761 [details]
Bug 1295084 Part 2 - Move two Position methods into nsStyleImageLayers.

https://reviewboard.mozilla.org/r/74080/#review72046

In your commit message, the extended explanation (which is great BTW!) ends with this right now:
> So I move the two methods into nsStyleImageLayers to
> break the dependency.

It's not clear (without the context of this bug) *why you'd care* about breaking that dependency.

Please add something like this at the end:
 "(so that a later patch can move Position out of nsStyleImageLayers)"
...for the benefit of future people looking at this changeset in isolation and wondering about the goal here.

::: layout/style/nsStyleStruct.h:598
(Diff revision 1)
>      }
>    };
>  
> +  static bool IsPositionOfInitialValue(Position aPosition, LayerType aType);
> +
> +  static float GetPositionInitialValue(LayerType aType) {

Hmm, these feel like awkward names.

Also, I think it's important that the name express that this function is specific to a particular layer-type (which is why it lives in nsStyleImageLayers).

These names sound clearer to me:
 IsInitialPositionForLayerType()
 GetInitialPositionForLayerType()

What do you think? (I'll mark this as r- for now, simply because the patch is entirely about this rename, and it's worth another glance after we've settled on final names.)
Attachment #8784761 - Flags: review?(dholbert) → review-
Comment on attachment 8784762 [details]
Bug 1295084 Part 3 - Move nsStyleImageLayers::Position to mozilla::Position.

https://reviewboard.mozilla.org/r/74082/#review72048

Looks great! r=me, 2 nits:

::: layout/base/nsLayoutUtils.cpp:4152
(Diff revision 1)
>  
>  // (Helper for HasInitialObjectFitAndPosition, to check
>  // each "object-position" coord.)
> -typedef nsStyleImageLayers::Position::PositionCoord PositionCoord;
>  static bool
> -IsCoord50Pct(const PositionCoord& aCoord)
> +IsCoord50Pct(const mozilla::Position::PositionCoord& aCoord)

This is in a .cpp file, and it has (or could have) "using namespace mozilla" up top. [OK, just checked -- this file does have a "using" decl already. Hooray!]

So - please drop "mozilla::" prefix here, as it shouldn't be necessary.

::: layout/base/nsLayoutUtils.cpp:4164
(Diff revision 1)
>  // Indicates whether the given nsStylePosition has the initial values
>  // for the "object-fit" and "object-position" properties.
>  static bool
>  HasInitialObjectFitAndPosition(const nsStylePosition* aStylePos)
>  {
> -  const nsStyleImageLayers::Position& objectPos = aStylePos->mObjectPosition;
> +  const mozilla::Position& objectPos = aStylePos->mObjectPosition;

Here as well, please drop mozilla:: prefix.
Attachment #8784762 - Flags: review?(dholbert) → review+
Comment on attachment 8784763 [details]
Bug 1295084 Part 4 - Rename Position::PositionCoord to Position::Coord.

https://reviewboard.mozilla.org/r/74084/#review72058

r=me
Attachment #8784763 - Flags: review?(dholbert) → review+
(Thanks CJ & TYLin for filing/fixing this bug, btw!)
Comment on attachment 8784761 [details]
Bug 1295084 Part 2 - Move two Position methods into nsStyleImageLayers.

https://reviewboard.mozilla.org/r/74080/#review72046

You're right. Adding explanation like this makes the commit message clearer.

> Hmm, these feel like awkward names.
> 
> Also, I think it's important that the name express that this function is specific to a particular layer-type (which is why it lives in nsStyleImageLayers).
> 
> These names sound clearer to me:
>  IsInitialPositionForLayerType()
>  GetInitialPositionForLayerType()
> 
> What do you think? (I'll mark this as r- for now, simply because the patch is entirely about this rename, and it's worth another glance after we've settled on final names.)

The two names are better than my proposal and sounds good to me to. Let's use them.
Comment on attachment 8784761 [details]
Bug 1295084 Part 2 - Move two Position methods into nsStyleImageLayers.

https://reviewboard.mozilla.org/r/74080/#review72316

r=me, with one nit:

::: layout/style/nsComputedDOMStyle.cpp:6151
(Diff revision 2)
>    if (svg->mMask.mImageCount > 1 ||
>        firstLayer.mClip != NS_STYLE_IMAGELAYER_CLIP_BORDER ||
>        firstLayer.mOrigin != NS_STYLE_IMAGELAYER_ORIGIN_BORDER ||
>        firstLayer.mComposite != NS_STYLE_MASK_COMPOSITE_ADD ||
>        firstLayer.mMaskMode != NS_STYLE_MASK_MODE_MATCH_SOURCE ||
> -      !firstLayer.mPosition.IsInitialValue(nsStyleImageLayers::LayerType::Mask) ||
> +      !nsStyleImageLayers::IsInitialPositionForLayerType(firstLayer.mPosition, nsStyleImageLayers::LayerType::Mask) ||

Please add a linebreak after the comma so this line isn't quite so long.
Attachment #8784761 - Flags: review?(dholbert) → review+
Comment on attachment 8784761 [details]
Bug 1295084 Part 2 - Move two Position methods into nsStyleImageLayers.

https://reviewboard.mozilla.org/r/74080/#review72316

> Please add a linebreak after the comma so this line isn't quite so long.

The second line will still be long if the line break is after the comma. I'll break the line after "(" so that the second line fits in 80 columns. Thanks.
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f4b25ed21f72
Part 1 - Move FragmentOrURL into mozilla namespace. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/0ee9a766e25a
Part 2 - Move two Position methods into nsStyleImageLayers. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/8e023decc250
Part 3 - Move nsStyleImageLayers::Position to mozilla::Position. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/25037d507662
Part 4 - Rename Position::PositionCoord to Position::Coord. r=dholbert
Yup, that's better. thanks!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: