Closed Bug 1456471 Opened Last year Closed Last year

Unify MediaList and ServoMediaList.

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files)

No description provided.
Comment on attachment 8970537 [details]
Bug 1456471: Unify MediaList and ServoMediaList.

https://reviewboard.mozilla.org/r/239302/#review244990


Code analysis found 2 defects in this patch:
 - 2 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/style/MediaList.cpp:80
(Diff revision 1)
> +  RefPtr<MediaList> clone =
> +    new MediaList(Servo_MediaList_DeepClone(mRawList).Consume());
> +  return clone.forget();
> +}
> +
> +MediaList::MediaList()

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

 0:10.91 MediaList::MediaList()
 0:10.91            ^
 0:10.91 Warning: modernize-use-auto in /cache/sa-central/layout/style/ServoBindings.cpp: use auto when initializing with a cast to avoid duplicating the type name

::: layout/style/MediaList.cpp:80
(Diff revision 1)
> +  RefPtr<MediaList> clone =
> +    new MediaList(Servo_MediaList_DeepClone(mRawList).Consume());
> +  return clone.forget();
> +}
> +
> +MediaList::MediaList()

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

 0:10.96 MediaList::MediaList()
 0:10.96            ^
 0:10.96 Warning: modernize-use-auto in /cache/sa-central/layout/style/ServoBindings.cpp: use auto when initializing with a cast to avoid duplicating the type name
(In reply to Code Review Bot [:reviewbot] from comment #3)
> Warning: Use '= default' to define a trivial default constructor
> [clang-tidy: modernize-use-equals-default]
> 
>  0:10.91 MediaList::MediaList()
>  0:10.91            ^
>  0:10.91 Warning: modernize-use-auto in
> /cache/sa-central/layout/style/ServoBindings.cpp: use auto when initializing
> with a cast to avoid duplicating the type name

You're drunk reviewbot, that wouldn't be equivalent.
Comment on attachment 8970537 [details]
Bug 1456471: Unify MediaList and ServoMediaList.

https://reviewboard.mozilla.org/r/239302/#review245184

::: layout/style/MediaList.h:39
(Diff revision 1)
> +  MediaList(const nsAString& aMedia, dom::CallerType);
> +  MediaList();

Consider moving them down into the `protected` section. Given they are not public interface of this class, it's probably better not to list them before the public ones.

::: layout/style/MediaList.h:55
(Diff revision 1)
> +
>    static already_AddRefed<MediaList> Create(const nsAString& aMedia,
>                                              CallerType aCallerType =
>                                                CallerType::NonSystem);
>  
> -  virtual already_AddRefed<MediaList> Clone() = 0;
> +  already_AddRefed<dom::MediaList> Clone();

You don't need to add `dom::` prefix here.

::: layout/style/MediaList.h:62
(Diff revision 1)
>    JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) final;
>    nsISupports* GetParentObject() const { return nullptr; }
>  
> -  virtual void GetText(nsAString& aMediaText) = 0;
> -  virtual void SetText(const nsAString& aMediaText) = 0;
> -  virtual bool Matches(nsPresContext* aPresContext) const = 0;
> +  void GetText(nsAString& aMediaText);
> +  void SetText(const nsAString& aMediaText);
> +  void SetTextInternal(const nsAString& aMediaText, CallerType);

This should still be put in `protected` section I suppose.

::: layout/style/MediaList.cpp:85
(Diff revision 1)
> +MediaList::MediaList()
> +  : mRawList(Servo_MediaList_Create().Consume())
> +{
> +}
> +
> +MediaList::MediaList(const nsAString& aMedia, dom::CallerType aCallerType)

`dom::` prefix is not needed here.

::: layout/style/MediaList.cpp:118
(Diff revision 1)
>    GetText(aMediaText);
>  }
>  
>  void
> +MediaList::SetTextInternal(const nsAString& aMediaText,
> +                           dom::CallerType aCallerType)

ditto.
Attachment #8970537 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8970538 [details]
Bug 1456471: Remove nsCSSParser.h.

https://reviewboard.mozilla.org/r/239304/#review245192

Looks good, thanks!
Attachment #8970538 - Flags: review?(xidorn+moz) → review+
https://hg.mozilla.org/mozilla-central/rev/60cbb70f1912
https://hg.mozilla.org/mozilla-central/rev/ff274dd24756
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.