If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove the WindowsJumpLists.jsm depedency on hasHistoryEntries

NEW
Unassigned

Status

()

Firefox
General
3 years ago
2 years ago

People

(Reporter: mak, Unassigned)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 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+
(Reporter)

Updated

3 years ago
Flags: needinfo?(mmucci)
(Reporter)

Comment 1

3 years ago
Created attachment 8525271 [details]
MozReview Request: bz://1101478/mak
Attachment #8525271 - Flags: review?(jmathies)
(Reporter)

Comment 2

3 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)

Comment 4

3 years ago
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.
(Reporter)

Comment 5

3 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.
(Reporter)

Comment 6

3 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

3 years ago
Attachment #8525271 - Flags: review?(jmathies) → review+

Comment 7

3 years ago
https://reviewboard.mozilla.org/r/777/#review391

Ship It!

Comment 8

3 years ago
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.
(Reporter)

Comment 9

3 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.
(Reporter)

Comment 10

3 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

Comment 11

3 years ago
(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: 3 years ago
Resolution: --- → FIXED
Blocks: 1104918
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 → ---

Updated

3 years ago
Iteration: 36.3 → 37.2
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/8e5fce59af55
https://hg.mozilla.org/releases/mozilla-aurora/rev/7dfea2969d9e
status-firefox36: fixed → ---
status-firefox37: affected → ---
Target Milestone: Firefox 36 → ---

Updated

3 years ago
Iteration: 37.2 → 37.3
(Reporter)

Comment 16

3 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
(Reporter)

Updated

3 years ago
Iteration: 37.3 - 12 Jan → ---
(Reporter)

Comment 17

2 years ago
Comment on attachment 8525271 [details]
MozReview Request: bz://1101478/mak
Attachment #8525271 - Attachment is obsolete: true
Attachment #8618651 - Flags: review+
(Reporter)

Comment 18

2 years ago
Created attachment 8618651 [details]
MozReview Request: Bug 1101478 - Remove the WindowsJumpLists.jsm depedency on hasHistoryEntries. r=jimm
You need to log in before you can comment on or make changes to this bug.