Stops updating the status bubble position when the bubble is empty.

[Mac] Always sets the status bubble to 1pt wide when hidden.

When the status bubble is in state kBubbleHidden, its width is now always 1pt.
This prevents us from wasting time resizing and redrawing a hidden bubble.

BUG=454502
TEST=No visible impact.  Status bubble should continue to show when hovering over links and resize properly when the window size is changed.

Committed: https://crrev.com/9ec18e41d4a50ebb61849921602a4dae996f448d
Cr-Commit-Position: refs/heads/master@{#318790}

[rohitrao (ping after 24h), 2015-02-02 18:33:53]

rohitrao@chromium.org changed reviewers:
+ erikchen@chromium.org

[rohitrao (ping after 24h), 2015-02-02 18:33:53]

Still playing around with this, but I wanted to give you an idea of what I was
looking at.

I can't find a better proxy for "the bubble is uninteresting, don't bother
updating its position."  window_ is always non-nil, window_ is always attached
to a parent window, and status/url_text_ are always non-nil, just empty.

[erikchen, 2015-02-02 19:57:58]

On 2015/02/02 18:33:53, rohitrao wrote:
> Still playing around with this, but I wanted to give you an idea of what I was
> looking at.
> 
> I can't find a better proxy for "the bubble is uninteresting, don't bother
> updating its position."  window_ is always non-nil, window_ is always attached
> to a parent window, and status/url_text_ are always non-nil, just empty.

I think the proxy you chose is fine, as long as we make its assumptions
explicit.

Your optimization relies on exactly two assumptions: 
  1. Any time the evaluation of the conditional "[status_text_ length] == 0 &&
[url_text_ length] == 0" changes from false to true, the method
SetFrameAvoidingMouse() is called. 
  2. When the conditional "[status_text_ length] == 0 && [url_text_ length] ==
0" evaluates to false, the bubble isn't shown.

From my reading of the code, both assumptions are true. If you make all of this
logic explicit by adding comments, and using the same conditional in SetText(),
then the CL should be an easy review. :)

[rohitrao (ping after 24h), 2015-02-02 20:58:30]

I switched to using state_, because it turns out we're actually explicitly
tracking bubble visibility in that variable.  Also, I found cases where the
text_ variables were empty even when the bubble was still technically visible
(waiting to animate out).  In those cases, I think we're better off updating
position.

[rohitrao (ping after 24h), 2015-02-02 21:30:23]

And of course this fails unit_tests =)

[rohitrao (ping after 24h), 2015-02-03 18:58:38]

Not happy that I have to keep the text_ comparisons, but the unit tests fail
without them.  I suspect that the tests are either taking some shortcuts or
making some bad assumptions, but I don't want to spend much more time on this,
since there are a million other things to look at.

[erikchen, 2015-02-03 20:24:02]

On 2015/02/03 18:58:38, rohitrao wrote:
> Not happy that I have to keep the text_ comparisons, but the unit tests fail
> without them.  I suspect that the tests are either taking some shortcuts or
> making some bad assumptions, but I don't want to spend much more time on this,
> since there are a million other things to look at.

Are you asking for a review?

[rohitrao (ping after 24h), 2015-02-03 20:25:31]

> Are you asking for a review?

Yup.

[erikchen, 2015-02-03 20:34:31]

On 2015/02/03 20:25:31, rohitrao wrote:
> > Are you asking for a review?
> 
> Yup.

It's not clear to me that patch set 5 is functionally correct. Let's say
|state_| = kBubbleHidden, and both text fields are empty. We know that the
position is not correct, because we haven't been updating it. Someone comes
along and calls SetText(), which first calls UpdateSizeAndPosition(), which
no-ops, because the state hasn't changed, and then calls StartShowing(), which
updates the state.

It looks like reordering UpdateSizeAndPosition(); and StartShowing(); fixes this
problem. Please add explicit comments about the implicit assumptions of your
optimization.

[Andre, 2015-02-03 20:40:23]

andresantoso@chromium.org changed reviewers:
+ andresantoso@chromium.org

[Andre, 2015-02-03 20:40:23]

How about setting the window size to kWindowSizeDeterminedLater in
CalculateWindowFrame when state is hidden, instead of keeping it 1/3
of the window's width?
That way SetFrameAvoidingMouse will update the child window's origin but
not its size, which won't trigger a redraw.

[rohitrao (ping after 24h), 2015-03-02 20:23:12]

I've updated the CL to (try and?) enforce that the bubble's size is
kWindowSizeDeterminedLater whenever it is hidden.

I could not simply add this to CalculateWindowFrame(), because that method is
called to elide the URL based on the calculated window size, and eliding the
full URL when the bubble is hidden prevents the bubble from being shown properly
later.

I suspect that this can be cleaned up and simplified further, but this seems
like a good place to start.  In Instruments, CPU usage attributed to the
StatusBubbleMac when resizing drops from about 9% to less than 1%.

[erikchen, 2015-03-02 21:30:41]

On 2015/03/02 20:23:12, rohitrao wrote:
> I've updated the CL to (try and?) enforce that the bubble's size is
> kWindowSizeDeterminedLater whenever it is hidden.
> 
> I could not simply add this to CalculateWindowFrame(), because that method is
> called to elide the URL based on the calculated window size, and eliding the
> full URL when the bubble is hidden prevents the bubble from being shown
properly
> later.
> 
> I suspect that this can be cleaned up and simplified further, but this seems
> like a good place to start.  In Instruments, CPU usage attributed to the
> StatusBubbleMac when resizing drops from about 9% to less than 1%.

I agree that this logic could be cleaned up further, but your CL is a strict
improvement over the current behavior. lgtm.

[Andre, 2015-03-02 22:18:13]

On 2015/03/02 20:23:12, rohitrao wrote:
> I've updated the CL to (try and?) enforce that the bubble's size is
> kWindowSizeDeterminedLater whenever it is hidden.
> 
> I could not simply add this to CalculateWindowFrame(), because that method is
> called to elide the URL based on the calculated window size, and eliding the
> full URL when the bubble is hidden prevents the bubble from being shown
properly
> later.
> 
> I suspect that this can be cleaned up and simplified further, but this seems
> like a good place to start.  In Instruments, CPU usage attributed to the
> StatusBubbleMac when resizing drops from about 9% to less than 1%.

What kind of CPU usage is that remaining 1%? Seems like it could be even less,
closer to 0%?

[rohitrao (ping after 24h), 2015-03-02 22:37:31]

> What kind of CPU usage is that remaining 1%? Seems like it could be even less,
> closer to 0%?

Let me get exact numbers.  I think it was just time spent in setFrame:display:,
testing against the current frame and returning early.  It was small, but I
don't remember exactly how small.

And it was the percentage that Instruments displays, not actual CPU usage.

[rohitrao (ping after 24h), 2015-03-02 22:38:10]

The CQ bit was checked by rohitrao@chromium.org

[commit-bot: I haz the power, 2015-03-02 22:41:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/895713002/120001

[commit-bot: I haz the power, 2015-03-02 22:51:46]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-02 22:52:20]

Patchset 7 (id:??) landed as
https://crrev.com/9ec18e41d4a50ebb61849921602a4dae996f448d
Cr-Commit-Position: refs/heads/master@{#318790}