Optimize tab close button hover tracking

RESOLVED FIXED

Status

Camino Graveyard
Tabbed Browsing
--
enhancement
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Stuart Morgan, Assigned: Stuart Morgan)

Tracking

({fixed1.8.1.1})

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

11 years ago
In order to pay for the tracking rects I added for tab tooltips, I looked into optimizing the rects we already had. Since we are already tracking when the mouse is in a tab for highlighting, and there's no way to mouse over a tab close button without first moving the mouse into a tab, we don't really need to track mouseover of all the close buttons; we just need to track the close button for a given tab while the mouse is in that tab.  That gives us one less rect per visible tab, bringing us back to pre-tooltip levels.
(Assignee)

Comment 1

11 years ago
Created attachment 241447 [details] [diff] [review]
trunk version

Trunk version. Also does a bit of name cleanup, and adds logic to prevent resetting the tracking rect three times per frame change.
Attachment #241447 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #241447 - Flags: review?(aschulm)
(Assignee)

Comment 2

11 years ago
Created attachment 241449 [details] [diff] [review]
branch-esque version

I don't have a branch handy at the moment, so this is actually a pre-tab-scrolling trunk patch, which I'm hoping will still apply on the branch.
Attachment #241449 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #241449 - Flags: review?(aschulm)
(Assignee)

Comment 3

11 years ago
Comment on attachment 241447 [details] [diff] [review]
trunk version

Simon, would you mind reviewing this?  It is a present for you ;)
Attachment #241447 - Flags: review? → review?(sfraser_bugs)
(Assignee)

Comment 4

11 years ago
Comment on attachment 241449 [details] [diff] [review]
branch-esque version

(Clearing review flags for branch; it's the same approach.)
Attachment #241449 - Flags: review?(aschulm)
Attachment #241449 - Flags: review?

Comment 5

11 years ago
Comment on attachment 241447 [details] [diff] [review]
trunk version

To avoid having to override setFrame:, setFrameOrigin: and setFrameSize: can't you override resizeSubviewsWithOldSize: and do the tracking rect update there? Or you could register for frame changed notifications and use those.

Other than that, the patch seems ok.
Attachment #241447 - Flags: review?(sfraser_bugs) → review+
(Assignee)

Comment 6

11 years ago
(In reply to comment #5)
> (From update of attachment 241447 [details] [diff] [review] [edit])
> To avoid having to override setFrame:, setFrameOrigin: and setFrameSize: can't
> you override resizeSubviewsWithOldSize: and do the tracking rect update there?

Unfortunately resizeSubviewsWithOldSize: is only called for bounds (frame size) changes, not all frame changes, which isn't enough for the tracking rects.

> Or you could register for frame changed notifications and use those.

Wouldn't posting a notification per tab per frame change in something that gets resized as much as the tabs be a lot of overhead?

Given that, I'd like to leave that for later consideration and get the part we know is a perf win now.  Frame-setter overrides are one of the suggested ways of doing tracking rect updates in the docs, so it's not like it's especially hackish.
(Assignee)

Comment 7

11 years ago
Created attachment 243749 [details] [diff] [review]
unbitrotted trunk version

Branch version coming shortly.
Attachment #241447 - Attachment is obsolete: true
Attachment #243749 - Flags: superreview?(mikepinkerton)
Attachment #241447 - Flags: review?(aschulm)
(Assignee)

Comment 8

11 years ago
(In reply to comment #7)
> Branch version coming shortly.

Nevermind; branch version still applies cleanly.
Comment on attachment 243749 [details] [diff] [review]
unbitrotted trunk version

sr=pink
Attachment #243749 - Flags: superreview?(mikepinkerton) → superreview+
Will we need to apply an additional part of the trunk patch to the branch when tab scrolling lands on the branch?
Whiteboard: [needs checkin]
(Assignee)

Comment 11

11 years ago
Comment on attachment 243749 [details] [diff] [review]
unbitrotted trunk version

Crap, this version is missing the BrowserTabBarView changes.  I'll post the complete version tonight.

(In reply to comment #10)
> Will we need to apply an additional part of the trunk patch to the branch when
> tab scrolling lands on the branch?

No, the code is the same between branch and trunk; they just have different context.
Attachment #243749 - Attachment is obsolete: true
(Assignee)

Comment 12

11 years ago
Created attachment 243873 [details] [diff] [review]
correct trunk version

This has the changes that were missing from the trunk patch.

Comment 13

11 years ago
Checked in on 1.8branch and trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
Whiteboard: [needs checkin]
You need to log in before you can comment on or make changes to this bug.