Copy contents of ui/base/ios into ios/chrome/browser/ui/context_menu

Copy contents of ui/base/ios into ios/chrome/browser/ui/context_menu

Moving context menu code into ios/chrome/browser/ui as a part of
factoring link context menus out of BrowserViewController.
Step 1 of 3.

BUG=604727
Committed: https://crrev.com/b64b9b67671fa06be5e3cebb55bef4bc2467b908
Cr-Commit-Position: refs/heads/master@{#388800}

[Jackie Quinn, 2016-04-15 19:15:55]

Description was changed from

==========
Copy contents of ui/base/ios into ios/chrome/browser/ui/context_menu

Moving context menu code into ios/chrome/browser/ui as a part of
factoring link context menus out of BrowserViewController
==========

to

==========
Copy contents of ui/base/ios into ios/chrome/browser/ui/context_menu

Moving context menu code into ios/chrome/browser/ui as a part of
factoring link context menus out of BrowserViewController. 
Step 1 of 3.
==========

[Jackie Quinn, 2016-04-15 19:16:24]

jyquinn@chromium.org changed reviewers:
+ eugenebut@chromium.org

[Jackie Quinn, 2016-04-15 19:17:55]

Let me know if this seems like a reasonable strategy:
1) Copy into ios/chrome/browser/ui/context_menu
2) Make changes in ios_internal
3) Delete ui/base/ios

[Eugene But (OOO till 7-30), 2016-04-15 19:57:18]

This seems like a reasonable strategy.

https://codereview.chromium.org/1891863004/diff/80001/ios/chrome/BUILD.gn
File ios/chrome/BUILD.gn (right):

https://codereview.chromium.org/1891863004/diff/80001/ios/chrome/BUILD.gn#new...
ios/chrome/BUILD.gn:54:
"browser/ui/context_menu/cru_context_menu_controller_unittest.mm",
We don't use prefixes or namespaces in chrome// layer. So please drop cru_ for
all files.

https://codereview.chromium.org/1891863004/diff/80001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/context_menu/cru_context_menu_controller_unittest.mm
(right):

https://codereview.chromium.org/1891863004/diff/80001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/context_menu/cru_context_menu_controller_unittest.mm:39:
[holder appendItemWithTitle:@"foo"
Optional NIT: This looks like a bug in clang format. The following formatting is
better and perfectly fine with style guide:
[holder appendItemWithTitle:@"foo" action:^{
  *clickedPtr = YES;
}];

https://codereview.chromium.org/1891863004/diff/80001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/context_menu/cru_context_menu_controller_unittest.mm:53:
[holder appendItemWithTitle:@"foo"
Optional NIT: Ditto. Old format is perfectly fine according to Style Guide.

https://codereview.chromium.org/1891863004/diff/80001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/context_menu/cru_context_menu_holder.h (right):

https://codereview.chromium.org/1891863004/diff/80001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/context_menu/cru_context_menu_holder.h:14: @interface
ContextMenuHolder : NSObject
This header in included in crw_web_controller.mm is it unnecessary there?

[Jackie Quinn, 2016-04-15 20:38:07]

https://codereview.chromium.org/1891863004/diff/80001/ios/chrome/BUILD.gn
File ios/chrome/BUILD.gn (right):

https://codereview.chromium.org/1891863004/diff/80001/ios/chrome/BUILD.gn#new...
ios/chrome/BUILD.gn:54:
"browser/ui/context_menu/cru_context_menu_controller_unittest.mm",
On 2016/04/15 19:57:17, Eugene But wrote:
> We don't use prefixes or namespaces in chrome// layer. So please drop cru_ for
> all files.

Okay, done.

https://codereview.chromium.org/1891863004/diff/80001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/context_menu/cru_context_menu_controller_unittest.mm
(right):

https://codereview.chromium.org/1891863004/diff/80001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/context_menu/cru_context_menu_controller_unittest.mm:39:
[holder appendItemWithTitle:@"foo"
On 2016/04/15 19:57:17, Eugene But wrote:
> Optional NIT: This looks like a bug in clang format. The following formatting
is
> better and perfectly fine with style guide:
> [holder appendItemWithTitle:@"foo" action:^{
>   *clickedPtr = YES;
> }];

Done.

https://codereview.chromium.org/1891863004/diff/80001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/context_menu/cru_context_menu_controller_unittest.mm:53:
[holder appendItemWithTitle:@"foo"
On 2016/04/15 19:57:18, Eugene But wrote:
> Optional NIT: Ditto. Old format is perfectly fine according to Style Guide.

Done.

https://codereview.chromium.org/1891863004/diff/80001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/context_menu/cru_context_menu_holder.h (right):

https://codereview.chromium.org/1891863004/diff/80001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/context_menu/cru_context_menu_holder.h:14: @interface
ContextMenuHolder : NSObject
On 2016/04/15 19:57:18, Eugene But wrote:
> This header in included in crw_web_controller.mm is it unnecessary there?

Yes. Removing it.

[Eugene But (OOO till 7-30), 2016-04-15 20:50:43]

lgtm, please wait for Sylvain as he is the current owner of this code

[Jackie Quinn, 2016-04-15 21:48:43]

Added ContextMenuProvider protocol to ios/chrome/browser/ui/context_menu

[sdefresne, 2016-04-18 08:47:28]

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

[sdefresne, 2016-04-18 08:47:29]

lgtm

If you do not want to fix the test in that CL (so that it is just refactoring),
could you please do it as a followup. Thank you.

https://codereview.chromium.org/1891863004/diff/140001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/context_menu/context_menu_controller.mm (right):

https://codereview.chromium.org/1891863004/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/context_menu/context_menu_controller.mm:36: // Weak
underlying UIAlertController.
nit: This comment is redundant with "base::WeakNSObject<UIAlertController>".
Maybe remove?

https://codereview.chromium.org/1891863004/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/context_menu/context_menu_controller.mm:45: //
Implementation specific for iOS version.
nit: This comment is redundant, this file is in a directory named "ios", so
obviously it is iOS version. Maybe remove?

https://codereview.chromium.org/1891863004/diff/140001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/context_menu/context_menu_controller_unittest.mm
(right):

https://codereview.chromium.org/1891863004/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/context_menu/context_menu_controller_unittest.mm:21:
_menuController.reset([[ContextMenuController alloc] init]);
Another buggy test that forgot to call "PlaformTest::SetUp" ... Can you fix
while you're here, of better move this code in the constructor as recommended in
gtest FAQ:

When you need to write per-test set-up and tear-down logic, you have the choice
between using the test fixture constructor/destructor or SetUp()/TearDown(). The
former is usually preferred.

https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

[rohitrao (ping after 24h), 2016-04-18 11:22:20]

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

[rohitrao (ping after 24h), 2016-04-18 11:22:21]

Is there a bug link for this work?

[Jackie Quinn, 2016-04-19 14:18:43]

Description was changed from

==========
Copy contents of ui/base/ios into ios/chrome/browser/ui/context_menu

Moving context menu code into ios/chrome/browser/ui as a part of
factoring link context menus out of BrowserViewController. 
Step 1 of 3.
==========

to

==========
Copy contents of ui/base/ios into ios/chrome/browser/ui/context_menu

Moving context menu code into ios/chrome/browser/ui as a part of
factoring link context menus out of BrowserViewController. 
Step 1 of 3.

BUG=604727
==========

[Jackie Quinn, 2016-04-19 14:24:27]

https://codereview.chromium.org/1891863004/diff/140001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/context_menu/context_menu_controller.mm (right):

https://codereview.chromium.org/1891863004/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/context_menu/context_menu_controller.mm:36: // Weak
underlying UIAlertController.
On 2016/04/18 08:47:29, sdefresne wrote:
> nit: This comment is redundant with "base::WeakNSObject<UIAlertController>".
> Maybe remove?

Done.

https://codereview.chromium.org/1891863004/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/context_menu/context_menu_controller.mm:45: //
Implementation specific for iOS version.
On 2016/04/18 08:47:29, sdefresne wrote:
> nit: This comment is redundant, this file is in a directory named "ios", so
> obviously it is iOS version. Maybe remove?

Done.

https://codereview.chromium.org/1891863004/diff/140001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/context_menu/context_menu_controller_unittest.mm
(right):

https://codereview.chromium.org/1891863004/diff/140001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/context_menu/context_menu_controller_unittest.mm:21:
_menuController.reset([[ContextMenuController alloc] init]);
On 2016/04/18 08:47:29, sdefresne wrote:
> Another buggy test that forgot to call "PlaformTest::SetUp" ... Can you fix
> while you're here, of better move this code in the constructor as recommended
in
> gtest FAQ:
> 
> When you need to write per-test set-up and tear-down logic, you have the
choice
> between using the test fixture constructor/destructor or SetUp()/TearDown().
The
> former is usually preferred.
> 
>
https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...

No problem, done!

[Jackie Quinn, 2016-04-19 14:24:42]

On 2016/04/18 11:22:21, rohitrao wrote:
> Is there a bug link for this work?

There is now!

[Jackie Quinn, 2016-04-19 15:45:17]

The CQ bit was checked by jyquinn@chromium.org

[Jackie Quinn, 2016-04-19 15:45:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1891863004/#ps160001
(title: "Comment and test cleanup")

[commit-bot: I haz the power, 2016-04-19 15:45:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1891863004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1891863004/160001

[commit-bot: I haz the power, 2016-04-19 16:06:17]

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

[commit-bot: I haz the power, 2016-04-19 16:06:19]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Jackie Quinn, 2016-04-19 16:13:15]

The CQ bit was checked by jyquinn@chromium.org

[Jackie Quinn, 2016-04-19 16:13:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1891863004/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-04-19 16:13:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1891863004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1891863004/180001

[commit-bot: I haz the power, 2016-04-19 16:32:24]

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

[commit-bot: I haz the power, 2016-04-19 16:32:25]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Jackie Quinn, 2016-04-21 15:59:28]

The CQ bit was checked by jyquinn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 15:59:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1891863004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1891863004/200001

[Jackie Quinn, 2016-04-21 16:00:50]

The CQ bit was unchecked by jyquinn@chromium.org

[Jackie Quinn, 2016-04-21 16:01:33]

The CQ bit was checked by jyquinn@chromium.org

[Jackie Quinn, 2016-04-21 16:01:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1891863004/#ps220001
(title: "Fix HardwardKeyboard test")

[commit-bot: I haz the power, 2016-04-21 16:01:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1891863004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1891863004/220001

[commit-bot: I haz the power, 2016-04-21 17:20:36]

Description was changed from

==========
Copy contents of ui/base/ios into ios/chrome/browser/ui/context_menu

Moving context menu code into ios/chrome/browser/ui as a part of
factoring link context menus out of BrowserViewController. 
Step 1 of 3.

BUG=604727
==========

to

==========
Copy contents of ui/base/ios into ios/chrome/browser/ui/context_menu

Moving context menu code into ios/chrome/browser/ui as a part of
factoring link context menus out of BrowserViewController. 
Step 1 of 3.

BUG=604727
==========

[commit-bot: I haz the power, 2016-04-21 17:20:37]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-04-22 19:35:40]

Description was changed from

==========
Copy contents of ui/base/ios into ios/chrome/browser/ui/context_menu

Moving context menu code into ios/chrome/browser/ui as a part of
factoring link context menus out of BrowserViewController. 
Step 1 of 3.

BUG=604727
==========

to

==========
Copy contents of ui/base/ios into ios/chrome/browser/ui/context_menu

Moving context menu code into ios/chrome/browser/ui as a part of
factoring link context menus out of BrowserViewController.
Step 1 of 3.

BUG=604727

Committed: https://crrev.com/b64b9b67671fa06be5e3cebb55bef4bc2467b908
Cr-Commit-Position: refs/heads/master@{#388800}
==========

[commit-bot: I haz the power, 2016-04-22 19:35:41]

Patchset 12 (id:??) landed as
https://crrev.com/b64b9b67671fa06be5e3cebb55bef4bc2467b908
Cr-Commit-Position: refs/heads/master@{#388800}