1101478 - Remove the WindowsJumpLists.jsm depedency on hasHistoryEntries

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: MozReview Request: bz://1101478/mak

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Pull down this commit:

hg pull review -r 70a9d7a7d4d302ba30dd88d9af01ac61537191ce

Comment 3

[Marco Mucci [:MarcoM]]

Added to IT 36.3

Comment 4

[Jim Mathies [:jimm]]

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.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(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.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 7

[Jim Mathies [:jimm]]

https://reviewboard.mozilla.org/r/777/#review391

Ship It!

Comment 8

[Jim Mathies [:jimm]]

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.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

while there i fixed some other minor trailing spaces too

https://hg.mozilla.org/integration/fx-team/rev/fc5225b5022b

Comment 11

[Jim Mathies [:jimm]]

(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.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fc5225b5022b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/8e5fce59af55

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7dfea2969d9e

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Bug 1101478 - Remove the WindowsJumpLists.jsm depedency on hasHistoryEntries.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

[:Gijs (he/him)]

P2 to at least investigate per the previous comments.

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I'm re-testing the patch on Try.

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I retriggered it a ton of times on Try and didn't see any crashes

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8618651 [details]
Bug 1101478 - Remove the WindowsJumpLists.jsm depedency on hasHistoryEntries.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/779/diff/1-2/

Comment 25

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69f07bbe86f9f395d7a9b0377bb79ed8f61c2b73

Comment 26

[Jim Mathies [:jimm]]

Comment on attachment 8618651 [details]
Bug 1101478 - Remove the WindowsJumpLists.jsm depedency on hasHistoryEntries.

https://reviewboard.mozilla.org/r/779/#review248062

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1a79ae86c49
Remove the WindowsJumpLists.jsm depedency on hasHistoryEntries. r=jimm

Comment 28

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e1a79ae86c49