Bug 66209 – Chromium Mac: Show scrollbar when doing search

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sailesh Agrawal]

Chromium Mac: Show scrollbar when doing search

[Sailesh Agrawal]

Created attachment 103884 [details]
Patch

[Sailesh Agrawal]

The chromium bug for this is:
http://code.google.com/p/chromium/issues/detail?id=92126

[Nico Weber]

Comment on attachment 103884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103884&action=review

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.h:75
> +    void paintTickmarks(GraphicsContext*, Scrollbar*, const IntRect&, const Vector<IntRect>&);

I have a feeling this will trigger clang's -Woverloaded-virtual. Maybe give it a different name?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2181
> +              scrollbar->invalidate();

Should you invalidate just the visibleHeight() when no tickmarks are visible? It looks like we repaint the entire scroll area all the time now (and even if there are tickmarks, invalidating just a scrollbarThickness() vertical slice should be enough)

[Sailesh Agrawal]

Comment on attachment 103884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103884&action=review

>> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.h:75
>> +    void paintTickmarks(GraphicsContext*, Scrollbar*, const IntRect&, const Vector<IntRect>&);
> 
> I have a feeling this will trigger clang's -Woverloaded-virtual. Maybe give it a different name?

Fixed. Renamed to paintGivenTickmarks()

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:2181
>> +              scrollbar->invalidate();
> 
> Should you invalidate just the visibleHeight() when no tickmarks are visible? It looks like we repaint the entire scroll area all the time now (and even if there are tickmarks, invalidating just a scrollbarThickness() vertical slice should be enough)

scrollbar->invalidate() only invalidates the widget. In this case the widget height is less than or equal to frame view's visible height. The widget's width is always equal to scrollbar thickness.

[Sailesh Agrawal]

Created attachment 103944 [details]
Patch

[Sailesh Agrawal]

change looks ok on try bots:
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds/273
http://build.chromium.org/p/tryserver.chromium/builders/win_layout_rel/builds/436
http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/1149

[Nico Weber]

(In reply to comment #4)
> scrollbar->invalidate() only invalidates the widget. In this case the widget height is less than or equal to frame view's visible height. The widget's width is always equal to scrollbar thickness.

Is the scrollbar height equal to the web height or the knob height usually? In the former case, invalidating less when there are no tickmarks would still be useful, right?

[James Robinson]

Why no test?

[Sailesh Agrawal]

(In reply to comment #7)
> (In reply to comment #4)
> > scrollbar->invalidate() only invalidates the widget. In this case the widget height is less than or equal to frame view's visible height. The widget's width is always equal to scrollbar thickness.
> 
> Is the scrollbar height equal to the web height or the knob height usually? In the former case, invalidating less when there are no tickmarks would still be useful, right?

The scrollbar widget height is just the visible page height. It's the least we can invalidate without looking up the tickmarks.

[Nico Weber]

Yes, but if there are no tickmarks, only the knob needs to be invalidated, right? Checking if the "are there tickmarks?" state matches the previous "are there tickmarks?" state and doing a smaller invalidation if it matches and if the state is "no tickmarks" should be easy, right?

[Sailesh Agrawal]

(In reply to comment #10)
> Yes, but if there are no tickmarks, only the knob needs to be invalidated, right?

I don't think that's true. For example, if we're showing tickmarks, and then the user cancels the search the new state is "no tickmarks". At this point we still want to invalidate the whole scrollbar widget since we want to erase the old tickmarks.

[Nico Weber]

That's why I said one would only do this if the old and new "are tickmarks shown?" state matches.

[Sailesh Agrawal]

(In reply to comment #10)
> Yes, but if there are no tickmarks, only the knob needs to be invalidated, right?

Hm.. there are four calls to WebFrameImpl::invalidateArea(). As far as I can tell tell they all call invalidateArea because the number of matches has changed.

Also, I can't see why invalidating just the knob would be useful. If the number of matches before and after is 0 then we shouldn't invalidate anything right? (Since the tickmarks are drawn on the track and possibly underneath the knob.)

For reference, I'm assuming that knob means that part of the scrollbar that you click and drag around and track means that background that the knob is dragged over.

[Nico Weber]

(In reply to comment #13)
> Also, I can't see why invalidating just the knob would be useful. If the number of matches before and after is 0 then we shouldn't invalidate anything right? (Since the tickmarks are drawn on the track and possibly underneath the knob.)

I'm thinking of the "scrolling without findmarks, with a normal, non-overlay scrollbar" case.

[Sailesh Agrawal]

(In reply to comment #14)
> (In reply to comment #13)
> > Also, I can't see why invalidating just the knob would be useful. If the number of matches before and after is 0 then we shouldn't invalidate anything right? (Since the tickmarks are drawn on the track and possibly underneath the knob.)
> 
> I'm thinking of the "scrolling without findmarks, with a normal, non-overlay scrollbar" case.

Ahh, got it. The good news is that this function is internal to this class and it's only used for search. The invalidation due to scrolling happens elsewhere.

[Nico Weber]

Ah, ok.

Can you add "No tests because there are no chromium 10.7 bots yet" to the changelog entry? with that, lgtm

[Sailesh Agrawal]

Created attachment 104408 [details]
Patch

[Sailesh Agrawal]

> Can you add "No tests because there are no chromium 10.7 bots yet" to the changelog entry? with that, lgtm

Done.

I also made one other change. The original patch would force both horizontal and vertical scrollbars to be visible when doing a search. With this change we only force the vertical scrollbar to be visible.

[Nico Weber]

Comment on attachment 104408 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104408&action=review

lgtm

> Source/WebCore/ChangeLog:10
> +        No tests because there are no chromium 10.7 bots yet.

nit: "Mac 10.7 chromium bots"

[James Robinson]

Comment on attachment 104408 [details]
Patch

I am rubber stamp hear me roar

[Sailesh Agrawal]

Created attachment 104409 [details]
Patch

[Sailesh Agrawal]

(In reply to comment #20)
> (From update of attachment 104408 [details])
> I am rubber stamp hear me roar

argh, and I just blew away the rubber stamp

[Sailesh Agrawal]

Created attachment 104410 [details]
Patch

[Nico Weber]

Comment on attachment 104410 [details]
Patch

no need for r?, has been reviewed already

[WebKit Review Bot]

Comment on attachment 104410 [details]
Patch

Clearing flags on attachment: 104410

Committed r93361: <http://trac.webkit.org/changeset/93361>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.