Closed
Bug 1129978
Opened 9 years ago
Closed 9 years ago
Replace custom addVisits() functions in the code base with the one in PlacesTestUtils.jsm
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: ttaubert, Assigned: ttaubert)
References
Details
Attachments
(3 files)
36.29 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
26.77 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
232.22 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
promiseAddVisits() should be replaced as well.
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8560060 -
Flags: review?(mak77)
Assignee | ||
Comment 4•9 years ago
|
||
Replaced all occurrences with sed. Ran xpcshell tests in toolkit/components/places to ensure this works.
Attachment #8560061 -
Flags: review?(mak77)
Assignee | ||
Updated•9 years ago
|
Iteration: --- → 38.2 - 9 Feb
Points: --- → 1
Flags: qe-verify-
Flags: firefox-backlog+
Assignee | ||
Comment 5•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=116719c0d28d
Comment 6•9 years ago
|
||
Comment on attachment 8560059 [details] [diff] [review] 0001-Bug-1129978-Replace-custom-addVisits-implementations.patch Review of attachment 8560059 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/thumbnails/test/head.js @@ +9,5 @@ > Cu.import("resource://gre/modules/FileUtils.jsm", tmp); > Cu.import("resource://gre/modules/osfile.jsm", tmp); > +Cu.import("resource://testing-common/PlacesTestUtils.jsm", tmp); > +let {PageThumbs, BackgroundPageThumbs, NewTabUtils, PageThumbsStorage, > + SessionStore, FileUtils, OS, PlacesTestUtils} = tmp; I'm not sure why this is importing into tmp rather than just using defineLazyModuleGetter. The result is even better (you get the laziness and don't leak the symbol). oh well... @@ +14,2 @@ > > Cu.import("resource://gre/modules/PlacesUtils.jsm"); This was only used by AddVisits, so you can remove it.
Attachment #8560059 -
Flags: review?(mak77) → review+
Updated•9 years ago
|
Attachment #8560060 -
Flags: review?(mak77) → review+
Comment 7•9 years ago
|
||
Comment on attachment 8560061 [details] [diff] [review] 0003-Bug-1129978-sed-i-s-promiseAddVisits-PlacesTestUtils.patch Review of attachment 8560061 [details] [diff] [review]: ----------------------------------------------------------------- I have only a big nit about the patches, that is indentation changes, stuff like: PlacesTestUtils.addVisits({ uri: something, transition: ... But there are many point where now it's wrong so, up to you. It's going to be boring as hell. I think it's more important to land stuff before it bitrots.
Attachment #8560061 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #7) > I have only a big nit about the patches, that is indentation changes, stuff > like: > PlacesTestUtils.addVisits({ uri: something, > transition: ... > But there are many point where now it's wrong so, up to you. It's going to > be boring as hell. It was, but indentation is fixed now :) https://hg.mozilla.org/integration/fx-team/rev/19e59777e22e https://hg.mozilla.org/integration/fx-team/rev/7d614e61d099 https://hg.mozilla.org/integration/fx-team/rev/f2cbc477d036
Updated•9 years ago
|
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/19e59777e22e https://hg.mozilla.org/mozilla-central/rev/7d614e61d099 https://hg.mozilla.org/mozilla-central/rev/f2cbc477d036
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox38:
--- → fixed
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
You need to log in
before you can comment on or make changes to this bug.
Description
•