Remove Tabpose feature on mac, and supporting infrastructure (PaintAtSize)

Remove Tabpose feature on mac, and supporting infrastructure (PaintAtSize)

Tabpose has been behind a flag for 3 years, and UMA shows usage is very low.
It is the unique user of the RenderWidgetSnapshotTaker, and the only user of
RWH::PaintAtSize, which we want to remove.

XIB changes:
* Remove Tabpose menu entry "Show Tab Overview..."

BUG=251960, 223336
R=cevans@chromium.org, jamesr@chromium.org, sky@chromium.org, viettrungluu@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251439

[piman, 2014-02-12 01:33:34]

I want to throw it out there.
Updating the feature to use some common snapshotting infrastructure (growing the
common infrastructure to reproduce the exact feature set) is definitely more
work than removing the feature, which is behind a flag and has very low UMA
usage.
git stats:  46 files changed, 2570 deletions(-)

Thoughts?

(note: this CL doesn't remove the menu entry from the MainMenu.xib, because
instructions suggests I'd need to reimage my machine back to 10.8. If decide to
move forward, I may need your help here).

[Nico, 2014-02-12 15:56:31]

I think this is the right thing to do. I considered it a few times as well, for
the same reasons you state (Well, I don't care too much about PaintAtSize, it's
more that it adds some general maintenance burden, I don't really maintain the
code, it's been behind a flag for very long and I didn't find time to turn it on
by default despite getting the ok for that over a year ago and nobody else did
either, etc. Also, it sets a horrible precedent for about:flags – this was one
of the first entries on there and stuff shouldn't stay behind a flag for several
years.)

So I'm supportive of the change. I can look at the code in a few days, or you
can get someone else to stamp – I assume it's an easy-to-review "delete lots of
code" .

Thanks!

[Nico, 2014-02-12 16:34:58]

On 2014/02/12 01:33:34, piman wrote:
> I want to throw it out there.
> Updating the feature to use some common snapshotting infrastructure (growing
the
> common infrastructure to reproduce the exact feature set) is definitely more
> work than removing the feature, which is behind a flag and has very low UMA
> usage.
> git stats:  46 files changed, 2570 deletions(-)
> 
> Thoughts?
> 
> (note: this CL doesn't remove the menu entry from the MainMenu.xib, because
> instructions suggests I'd need to reimage my machine back to 10.8. If decide
to
> move forward, I may need your help here).

(Bug for the IB stuff is
https://code.google.com/p/chromium/issues/detail?id=327347 which has a somewhat
annoying blocker. I'm looking at this.)

[piman, 2014-02-12 23:22:39]

Ok, I added the XIB changes to remove the menu entry. I /think/ I didn't screw
it up.

This CL only deletes things. No sugar added.

rsesek: please review the mac bits.
jamesr: please review the content/ bits
sky: OWNER for chrome/
cevans: security for view_messages.h

[Chris Evans, 2014-02-12 23:25:16]

On 2014/02/12 23:22:39, piman wrote:
> Ok, I added the XIB changes to remove the menu entry. I /think/ I didn't screw
> it up.
> 
> This CL only deletes things. No sugar added.
> 
> rsesek: please review the mac bits.
> jamesr: please review the content/ bits
> sky: OWNER for chrome/
> cevans: security for view_messages.h

codereview upload issues?

[piman, 2014-02-12 23:31:07]

On Wed, Feb 12, 2014 at 3:25 PM, <cevans@chromium.org> wrote:

> On 2014/02/12 23:22:39, piman wrote:
>
>> Ok, I added the XIB changes to remove the menu entry. I /think/ I didn't
>> screw
>> it up.
>>
>
>  This CL only deletes things. No sugar added.
>>
>
>  rsesek: please review the mac bits.
>> jamesr: please review the content/ bits
>> sky: OWNER for chrome/
>> cevans: security for view_messages.h
>>
>
> codereview upload issues?
>

GAE is acting up. Try reloading... It should be at PS6 or 7, but different
GAE instances give different views. Sigh.


>
> https://codereview.chromium.org/154083008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jamesr, 2014-02-13 00:00:47]

content/ lgtm

[Chris Evans, 2014-02-13 00:53:07]

On 2014/02/13 00:00:47, jamesr wrote:
> content/ lgtm

view_messages.h lgtm

[sky, 2014-02-13 01:35:36]

Can you try uploading again, not all side-by-side diffs made it.

On Wed, Feb 12, 2014 at 4:53 PM,  <cevans@chromium.org> wrote:
> On 2014/02/13 00:00:47, jamesr wrote:
>>
>> content/ lgtm
>
>
> view_messages.h lgtm
>
> https://codereview.chromium.org/154083008/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-02-13 01:49:34]

On 2014/02/13 01:35:36, sky wrote:
> Can you try uploading again, not all side-by-side diffs made it.
> 
> On Wed, Feb 12, 2014 at 4:53 PM,  <mailto:cevans@chromium.org> wrote:
> > On 2014/02/13 00:00:47, jamesr wrote:
> >>
> >> content/ lgtm
> >
> >
> > view_messages.h lgtm
> >
> > https://codereview.chromium.org/154083008/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I keep getting 500's sorry.... I tried 3 times.
The unified diff is there, though.

[sky, 2014-02-13 16:59:51]

LGTM

https://codereview.chromium.org/154083008/diff/600001/chrome/app/chrome_comma...
File chrome/app/chrome_command_ids.h (left):

https://codereview.chromium.org/154083008/diff/600001/chrome/app/chrome_comma...
chrome/app/chrome_command_ids.h:64: #define IDC_TABPOSE                    
34036
Is there a reason why we don't collapse ids when we remove constants?

[piman, 2014-02-13 19:11:49]

https://codereview.chromium.org/154083008/diff/600001/chrome/app/chrome_comma...
File chrome/app/chrome_command_ids.h (left):

https://codereview.chromium.org/154083008/diff/600001/chrome/app/chrome_comma...
chrome/app/chrome_command_ids.h:64: #define IDC_TABPOSE                    
34036
On 2014/02/13 16:59:52, sky wrote:
> Is there a reason why we don't collapse ids when we remove constants?

I suppose, because we don't have a good script that does it, and also keeping
annotate info (this is akin to fixing style).

[piman, 2014-02-13 20:30:27]

-rsesek (who's away too) +viettrungluu who reviewed some patches that introduced
the feature

[viettrungluu, 2014-02-14 16:50:15]

On 2014/02/13 20:30:27, piman wrote:
> -rsesek (who's away too) +viettrungluu who reviewed some patches that
introduced
> the feature

LGTM AFAICT. Hopefully you got the xib changes right (it's hard to tell).

[Avi (use Gerrit), 2014-02-14 17:01:44]

https://codereview.chromium.org/154083008/diff/600001/content/public/browser/...
File content/public/browser/notification_types.h (left):

https://codereview.chromium.org/154083008/diff/600001/content/public/browser/...
content/public/browser/notification_types.h:185:
NOTIFICATION_RENDER_WIDGET_HOST_DID_RECEIVE_PAINT_AT_SIZE_ACK,
Yes! I'm adding another bug to your bug list, http://crbug.com/223336

[piman, 2014-02-14 19:13:34]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-02-14 19:14:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/154083008/600001

[commit-bot: I haz the power, 2014-02-14 19:47:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-14 19:47:07]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[piman, 2014-02-14 20:33:47]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-02-14 20:35:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/154083008/930001

[commit-bot: I haz the power, 2014-02-14 23:39:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-14 23:39:13]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[piman, 2014-02-14 23:45:30]

Committed patchset #11 manually as r251439 (presubmit successful).