Closed
Bug 1456471
Opened 6 years ago
Closed 6 years ago
Unify MediaList and ServoMediaList.
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
Details
Attachments
(2 files)
No description provided.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•6 years ago
|
||
mozreview-review |
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
Assignee | ||
Comment 4•6 years ago
|
||
(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 5•6 years ago
|
||
mozreview-review |
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 6•6 years ago
|
||
mozreview-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+
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/60cbb70f1912 Unify MediaList and ServoMediaList. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/ff274dd24756 Remove nsCSSParser.h. r=xidorn
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/60cbb70f1912 https://hg.mozilla.org/mozilla-central/rev/ff274dd24756
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•