Remove the WindowsJumpLists.jsm depedency on hasHistoryEntries

RESOLVED FIXED in Firefox 62

Status

()

RESOLVED FIXED
4 years ago
11 months ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
Firefox 62
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [fxperf:p2])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
The hasHistoryEntries usage there is not very mandatory.

the optimizations we make for the empty history case may affect a very minor percentage of users (permanent pb) for which the queries would be fast already.

The clear history on shutdown would be better served by doing an update on clear history, more generally.

Marco, please add this to the current iteration.
Flags: qe-verify-
Flags: firefox-backlog+
(Assignee)

Updated

4 years ago
Flags: needinfo?(mmucci)
(Assignee)

Comment 1

4 years ago
Posted file MozReview Request: bz://1101478/mak (obsolete) —
Attachment #8525271 - Flags: review?(jmathies)
(Assignee)

Comment 2

4 years ago
/r/779 - Bug 1101478 - Remove the WindowsJumpLists.jsm depedency on hasHistoryEntries. r=jimm

Pull down this commit:

hg pull review -r 70a9d7a7d4d302ba30dd88d9af01ac61537191ce
Added to IT 36.3
Flags: needinfo?(mmucci)
I haven't set up review board yet and have no idea how to work with it. I'll look into that but if you're interested in quick turn around time please post a normal dif for review.
(Assignee)

Comment 5

4 years ago
(In reply to Jim Mathies [:jimm] from comment #4)
> I haven't set up review board yet and have no idea how to work with it. I'll
> look into that but if you're interested in quick turn around time please
> post a normal dif for review.

You don't have to setup anything, just enter the link with your user/password for bugzilla, go to the "diff" section and it basically works like Splinter. When you are done adding comments, go to the "reviews" section, choose publish and mark the Ship it! check box if it's a plus.

I actually had your same doubts when I got my first request with RB, but it took just a few minutes, not much different than Splinter.
(Assignee)

Comment 6

4 years ago
in case of doubts, this should help with the UI:
https://www.reviewboard.org/docs/manual/2.0/users/reviews/reviewing-diffs/

Updated

4 years ago
Attachment #8525271 - Flags: review?(jmathies) → review+
https://reviewboard.mozilla.org/r/777/#review393

I can't seem to get the splinter interface working. you have white space cleanup you need to do in one spot.
(Assignee)

Comment 9

4 years ago
just clicking once on the line NUMBER should allow you to add a comment through a popup (And that opens an issue for me to fix).

Btw, found the spot, thanks.
(Assignee)

Comment 10

4 years ago
while there i fixed some other minor trailing spaces too

https://hg.mozilla.org/integration/fx-team/rev/fc5225b5022b
Target Milestone: --- → Firefox 36
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #9)
> just clicking once on the line NUMBER should allow you to add a comment
> through a popup (And that opens an issue for me to fix).
> 
> Btw, found the spot, thanks.

Thanks. I think I was clicking on individual lines expecting a splinter inline comment panel to appear. Will try the line number trick next time.

All in all, looking forward to working with this.
https://hg.mozilla.org/mozilla-central/rev/fc5225b5022b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Backed out on inbound for suspicion of causing the spike in bug 1103966.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e5fce59af55

I'll back this out from Aurora as well once it's confirmed fixed on trunk.
Status: RESOLVED → REOPENED
status-firefox36: --- → fixed
status-firefox37: --- → affected
Resolution: FIXED → ---
Iteration: 36.3 → 37.2
https://hg.mozilla.org/releases/mozilla-aurora/rev/7dfea2969d9e
status-firefox36: fixed → ---
status-firefox37: affected → ---
Target Milestone: Firefox 36 → ---
Iteration: 37.2 → 37.3
(Assignee)

Comment 16

4 years ago
I'm unassigning this cause the benefit is almost nonexistent atm, while investigating the shutdown crash could take huge effort.

It's basically not worth it now.
Assignee: mak77 → nobody
Status: REOPENED → NEW
(Assignee)

Updated

4 years ago
Iteration: 37.3 - 12 Jan → ---
(Assignee)

Comment 17

4 years ago
Comment on attachment 8525271 [details]
MozReview Request: bz://1101478/mak
Attachment #8525271 - Attachment is obsolete: true
Attachment #8618651 - Flags: review+
BHR / arewesmoothyet.com indicates that there are a non-zero set of users experiencing hundreds of milliseconds worth of hangs with hasHistoryEntries on the stack due to main thread IO. I think we should revisit this.
Whiteboard: [fxperf]
(Assignee)

Comment 20

a year ago
The backout issue may be gone in the meanwhile, I just never had a chance to check if this still causes bug 1103966.
Apart from the NetUtil usage the original patch should still apply.

Comment 21

a year ago
P2 to at least investigate per the previous comments.
Whiteboard: [fxperf] → [fxperf:p2]
(Assignee)

Comment 22

11 months ago
I'm re-testing the patch on Try.
(Assignee)

Comment 23

11 months ago
I retriggered it a ton of times on Try and didn't see any crashes
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 26

11 months ago
mozreview-review
Comment on attachment 8618651 [details]
Bug 1101478 - Remove the WindowsJumpLists.jsm depedency on hasHistoryEntries.

https://reviewboard.mozilla.org/r/779/#review248062
Attachment #8618651 - Flags: review?(jmathies) → review+

Comment 27

11 months ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/e1a79ae86c49
Remove the WindowsJumpLists.jsm depedency on hasHistoryEntries. r=jimm

Comment 28

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e1a79ae86c49
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago11 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.