[ios] Creates ToolsMenuConstants file

[ios] Creates ToolsMenuConstants file

Since many tests depend on just these constants, I think is better to
create a separate file for these since the Model classes might change
again and we don't wan't to keep updating these tests.

This CL removes the model code from ToolsMenuVC and fixes a bug in 
ToolsMenuModel

Downstream tests compile and run successfully against these changes.

BUG=682880
Review-Url: https://codereview.chromium.org/2722693002
Cr-Commit-Position: refs/heads/master@{#455949}
Committed: https://chromium.googlesource.com/chromium/src/+/d4523043de7614ef55438aaa42fadafb905a0e56

[sczs, 2017-02-27 22:37:22]

Description was changed from

==========
[ios] Creates ToolsMenuConstants file
Since many test depend on just these constants, I think is better to
create a separate file for these since the Model classes might change
again and we don't wan't to keep updating these tests.

BUG=682880
==========

to

==========
[ios] Creates ToolsMenuConstants file

Since many tests depend on just these constants, I think is better to
create a separate file for these since the Model classes might change
again and we don't wan't to keep updating these tests.

Downstream tests compile and run successfully against these changes.

BUG=682880
==========

[sczs, 2017-02-27 22:37:22]

sczs@chromium.org changed reviewers:
+ edchin@chromium.org, marq@chromium.org, rohitrao@chromium.org

[sczs, 2017-02-27 22:38:31]

Description was changed from

==========
[ios] Creates ToolsMenuConstants file

Since many tests depend on just these constants, I think is better to
create a separate file for these since the Model classes might change
again and we don't wan't to keep updating these tests.

Downstream tests compile and run successfully against these changes.

BUG=682880
==========

to

==========
[ios] Creates ToolsMenuConstants file

Since many tests depend on just these constants, I think is better to
create a separate file for these since the Model classes might change
again and we don't wan't to keep updating these tests.

This CL removes the model code from ToolsMenuVC and fixes a bug in 
ToolsMenuModel

Downstream tests compile and run successfully against these changes.

BUG=682880
==========

[sczs, 2017-02-27 22:41:06]

PTAL

[edchin, 2017-02-27 23:02:41]

https://codereview.chromium.org/2722693002/diff/1/ios/chrome/browser/metrics/...
File ios/chrome/browser/metrics/tab_usage_recorder_egtest.mm (right):

https://codereview.chromium.org/2722693002/diff/1/ios/chrome/browser/metrics/...
ios/chrome/browser/metrics/tab_usage_recorder_egtest.mm:20: #include
"ios/chrome/browser/ui/tools_menu/tools_menu_constants.h"
Just out of curiosity, why #include instead of #import in this case?

https://codereview.chromium.org/2722693002/diff/1/ios/chrome/browser/ui/tools...
File ios/chrome/browser/ui/tools_menu/BUILD.gn (right):

https://codereview.chromium.org/2722693002/diff/1/ios/chrome/browser/ui/tools...
ios/chrome/browser/ui/tools_menu/BUILD.gn:9: "tools_menu_constants.h",
Would we later move this to //ios/shared?

https://codereview.chromium.org/2722693002/diff/1/ios/chrome/browser/ui/tools...
File ios/chrome/browser/ui/tools_menu/tools_menu_model.mm (left):

https://codereview.chromium.org/2722693002/diff/1/ios/chrome/browser/ui/tools...
ios/chrome/browser/ui/tools_menu/tools_menu_model.mm:96: return
!ios::GetChromeBrowserProvider()
Can you explain this change?

https://codereview.chromium.org/2722693002/diff/1/ios/chrome/browser/ui/tools...
File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm (right):

https://codereview.chromium.org/2722693002/diff/1/ios/chrome/browser/ui/tools...
ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:223:
context.hasNoOpenedTabs
It's odd that clang-format moved this line when nothing changed here.

[edchin, 2017-02-27 23:05:11]

lgtm % double check the change on
ios/chrome/browser/ui/tools_menu/tools_menu_model.mm:96.

[sczs, 2017-03-01 20:43:40]

Patchset #2 (id:20001) has been deleted

[sczs, 2017-03-01 20:44:54]

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

[commit-bot: I haz the power, 2017-03-01 20:45:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 20:52:48]

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

[commit-bot: I haz the power, 2017-03-01 20:52:49]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[sczs, 2017-03-02 02:12:20]

Patchset #2 (id:40001) has been deleted

[sczs, 2017-03-02 02:22:17]

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

[commit-bot: I haz the power, 2017-03-02 02:23:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[marq (ping after 24h), 2017-03-02 06:09:48]

lgtm

[commit-bot: I haz the power, 2017-03-02 06:12:28]

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

[commit-bot: I haz the power, 2017-03-02 06:12:29]

Dry run: This issue passed the CQ dry run.

[sczs, 2017-03-02 22:56:11]

Moved all constants to shared folder, updated the eg tests and their respective
BUILD.gn files

https://codereview.chromium.org/2722693002/diff/1/ios/chrome/browser/metrics/...
File ios/chrome/browser/metrics/tab_usage_recorder_egtest.mm (right):

https://codereview.chromium.org/2722693002/diff/1/ios/chrome/browser/metrics/...
ios/chrome/browser/metrics/tab_usage_recorder_egtest.mm:20: #include
"ios/chrome/browser/ui/tools_menu/tools_menu_constants.h"
On 2017/02/27 23:02:41, edchin wrote:
> Just out of curiosity, why #include instead of #import in this case?

You're suppose to import Obj-C headers and include C++ headers, though I think
it doesn't matter much here. 
https://google.github.io/styleguide/objcguide.xml#_import_and__include

https://codereview.chromium.org/2722693002/diff/1/ios/chrome/browser/ui/tools...
File ios/chrome/browser/ui/tools_menu/BUILD.gn (right):

https://codereview.chromium.org/2722693002/diff/1/ios/chrome/browser/ui/tools...
ios/chrome/browser/ui/tools_menu/BUILD.gn:9: "tools_menu_constants.h",
On 2017/02/27 23:02:41, edchin wrote:
> Would we later move this to //ios/shared?

Since I don't want to keep updating the tests I will move it now :) Thanks for
pointing this out.

https://codereview.chromium.org/2722693002/diff/1/ios/chrome/browser/ui/tools...
File ios/chrome/browser/ui/tools_menu/tools_menu_model.mm (left):

https://codereview.chromium.org/2722693002/diff/1/ios/chrome/browser/ui/tools...
ios/chrome/browser/ui/tools_menu/tools_menu_model.mm:96: return
!ios::GetChromeBrowserProvider()
On 2017/02/27 23:02:41, edchin wrote:
> Can you explain this change?

This is a bug I introduced when changing the if statement to this switch. The
downstream test caught it, and this is how it should've been from the start.

https://codereview.chromium.org/2722693002/diff/1/ios/chrome/browser/ui/tools...
File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm (right):

https://codereview.chromium.org/2722693002/diff/1/ios/chrome/browser/ui/tools...
ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:223:
context.hasNoOpenedTabs
On 2017/02/27 23:02:41, edchin wrote:
> It's odd that clang-format moved this line when nothing changed here.

I think the previous patch might have been uploaded when there were some
clang-format #of line issues going on, that's my guess.

[sczs, 2017-03-02 23:48:45]

sczs@chromium.org changed reviewers:
+ baxley@chromium.org

[sczs, 2017-03-02 23:55:12]

Rohit, Baxley,

Could you please take a look, I need your lgtm as owner of /metrics and /test

Thanks!

[baxley, 2017-03-03 16:51:11]

//ios/chrome/test LGTM

[sczs, 2017-03-03 22:36:04]

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

[sczs, 2017-03-03 22:36:05]

Hi Sylvain,

Could you please take a look at the changes at DEPS and BUILD.gn files and let
me know if you are OK with adding dependencies to ios/shared?

Thanks,
Sergio

[sdefresne, 2017-03-03 22:51:51]

code lgtm

I'm personally okay moving those files in ios/shared, but rohitrao is not happy
with the existence of ios/shared. Should we instead introduce the new files in
ios/chrome, and if it turns out we want to have ios/shared move them in a later
CL (it looks like they are only used from ios/chrome currently).

WDYT?

[sczs, 2017-03-09 18:21:31]

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

[commit-bot: I haz the power, 2017-03-09 18:22:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 18:25:01]

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

[commit-bot: I haz the power, 2017-03-09 18:25:02]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sczs, 2017-03-09 23:53:50]

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

[commit-bot: I haz the power, 2017-03-09 23:54:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sczs, 2017-03-10 00:07:48]

On 2017/03/03 22:51:51, sdefresne wrote:
> code lgtm
> 
> I'm personally okay moving those files in ios/shared, but rohitrao is not
happy
> with the existence of ios/shared. Should we instead introduce the new files in
> ios/chrome, and if it turns out we want to have ios/shared move them in a
later
> CL (it looks like they are only used from ios/chrome currently).
> 
> WDYT?

Sounds good, we can wait until we have a better sense on what goes into the
shared folder and decide then.

[commit-bot: I haz the power, 2017-03-10 00:51:38]

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

[commit-bot: I haz the power, 2017-03-10 00:51:39]

Dry run: This issue passed the CQ dry run.

[sczs, 2017-03-10 01:24:15]

The CQ bit was checked by sczs@chromium.org

[sczs, 2017-03-10 01:24:16]

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

[commit-bot: I haz the power, 2017-03-10 01:24:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 01:29:44]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489109055735130,
"parent_rev": "187cdc9728275a1d5120738442cc10d6566538d3", "commit_rev":
"d4523043de7614ef55438aaa42fadafb905a0e56"}

[commit-bot: I haz the power, 2017-03-10 01:30:21]

Description was changed from

==========
[ios] Creates ToolsMenuConstants file

Since many tests depend on just these constants, I think is better to
create a separate file for these since the Model classes might change
again and we don't wan't to keep updating these tests.

This CL removes the model code from ToolsMenuVC and fixes a bug in 
ToolsMenuModel

Downstream tests compile and run successfully against these changes.

BUG=682880
==========

to

==========
[ios] Creates ToolsMenuConstants file

Since many tests depend on just these constants, I think is better to
create a separate file for these since the Model classes might change
again and we don't wan't to keep updating these tests.

This CL removes the model code from ToolsMenuVC and fixes a bug in 
ToolsMenuModel

Downstream tests compile and run successfully against these changes.

BUG=682880

Review-Url: https://codereview.chromium.org/2722693002
Cr-Commit-Position: refs/heads/master@{#455949}
Committed:
https://chromium.googlesource.com/chromium/src/+/d4523043de7614ef55438aaa42fa...
==========

[commit-bot: I haz the power, 2017-03-10 01:30:22]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d4523043de7614ef55438aaa42fa...