Closed Bug 1101478 Opened 5 years ago Closed Last year

Remove the WindowsJumpLists.jsm depedency on hasHistoryEntries

Categories

(Firefox :: General, defect)

defect
Not set
Points:
1

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox62 --- fixed

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [fxperf:p2])

Attachments

(1 file, 1 obsolete file)

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+
Flags: needinfo?(mmucci)
Attached file MozReview Request: bz://1101478/mak (obsolete) —
Attachment #8525271 - Flags: review?(jmathies)
/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.
(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.
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.
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.
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
Closed: 5 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
Resolution: FIXED → ---
Iteration: 36.3 → 37.2
Iteration: 37.2 → 37.3
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
Iteration: 37.3 - 12 Jan → ---
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]
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.
P2 to at least investigate per the previous comments.
Whiteboard: [fxperf] → [fxperf:p2]
I'm re-testing the patch on Try.
I retriggered it a ton of times on Try and didn't see any crashes
Assignee: nobody → mak77
Status: NEW → ASSIGNED
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+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/e1a79ae86c49
Remove the WindowsJumpLists.jsm depedency on hasHistoryEntries. r=jimm
https://hg.mozilla.org/mozilla-central/rev/e1a79ae86c49
Status: ASSIGNED → RESOLVED
Closed: 5 years agoLast year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.