grit: Reorganize and compact resource_ids.

grit: Reorganize and compact resource_ids.

BUG=585301
Committed: https://crrev.com/12af52210b397d1d76c541730d95c7a0375496c4
Cr-Commit-Position: refs/heads/master@{#427236}

[Lei Zhang, 2016-10-20 06:15:28]

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

[commit-bot: I haz the power, 2016-10-20 06:15:43]

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

[Lei Zhang, 2016-10-20 06:41:36]

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

[commit-bot: I haz the power, 2016-10-20 06:41:52]

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

[Lei Zhang, 2016-10-20 07:17:40]

thestig@chromium.org changed reviewers:
+ flackr@chromium.org, sdefresne@chromium.org

[Lei Zhang, 2016-10-20 07:17:40]

sdefresne: Can you look at this from an iOS perspective and see if it's ok?
flackr: General review.

[commit-bot: I haz the power, 2016-10-20 07:46:48]

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

[commit-bot: I haz the power, 2016-10-20 07:46:48]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sdefresne, 2016-10-20 14:59:55]

https://codereview.chromium.org/2429213007/diff/40001/tools/gritsettings/reso...
File tools/gritsettings/resource_ids (right):

https://codereview.chromium.org/2429213007/diff/40001/tools/gritsettings/reso...
tools/gritsettings/resource_ids:18: # - ios/ (overlaps with content/)
This separation is a bit problematic for iOS. It works, but limit how many
resources we can use. Also the everything else is a bit strange that includes
things iOS depends (ui/, net/) and thing it does not depends (android, ash,
...).

As iOS can overlap with anything it does not depends on, could we have the
following sections (in that order, which I know is not alphabetic, but more
topological):

- net/
- ui/
- components/
- ios/ (can overlap all sections below)
- contents/
- chrome/app
- chrome/browser/
- chrome/ miscellaneous
- everything else

I think that as is, this will work as downstream code only use 31000-36000 for
it's idea, but it will make our life harder for upstreaming. The ordering I
suggest would make it easier (though it is probably much more work for you).

[Lei Zhang, 2016-10-20 20:53:56]

https://codereview.chromium.org/2429213007/diff/40001/tools/gritsettings/reso...
File tools/gritsettings/resource_ids (right):

https://codereview.chromium.org/2429213007/diff/40001/tools/gritsettings/reso...
tools/gritsettings/resource_ids:18: # - ios/ (overlaps with content/)
On 2016/10/20 14:59:55, sdefresne wrote:
> This separation is a bit problematic for iOS. It works, but limit how many
> resources we can use. Also the everything else is a bit strange that includes
> things iOS depends (ui/, net/) and thing it does not depends (android, ash,
> ...).
> 
> As iOS can overlap with anything it does not depends on, could we have the
> following sections (in that order, which I know is not alphabetic, but more
> topological):
> 
> - net/
> - ui/
> - components/
> - ios/ (can overlap all sections below)
> - contents/
> - chrome/app
> - chrome/browser/
> - chrome/ miscellaneous
> - everything else
> 
> I think that as is, this will work as downstream code only use 31000-36000 for
> it's idea, but it will make our life harder for upstreaming. The ordering I
> suggest would make it easier (though it is probably much more work for you).

Right, so I haven't worked downstream in a while and no longer have access to
the code, so no peeking for me.

If everything gets upstreamed, would it all go into ios/ or would some parts end
up elsewhere like in components/ ?

Maybe ios/ should not overlap with contents/ but overlap with chrome/ instead.
There is a huge chunk below id 10000, and that should be enough for everything
ios/.

[sdefresne, 2016-10-20 21:28:56]

On 2016/10/20 20:53:56, Lei Zhang wrote:
>
https://codereview.chromium.org/2429213007/diff/40001/tools/gritsettings/reso...
> File tools/gritsettings/resource_ids (right):
> 
>
https://codereview.chromium.org/2429213007/diff/40001/tools/gritsettings/reso...
> tools/gritsettings/resource_ids:18: # - ios/ (overlaps with content/)
> On 2016/10/20 14:59:55, sdefresne wrote:
> > This separation is a bit problematic for iOS. It works, but limit how many
> > resources we can use. Also the everything else is a bit strange that
includes
> > things iOS depends (ui/, net/) and thing it does not depends (android, ash,
> > ...).
> > 
> > As iOS can overlap with anything it does not depends on, could we have the
> > following sections (in that order, which I know is not alphabetic, but more
> > topological):
> > 
> > - net/
> > - ui/
> > - components/
> > - ios/ (can overlap all sections below)
> > - contents/
> > - chrome/app
> > - chrome/browser/
> > - chrome/ miscellaneous
> > - everything else
> > 
> > I think that as is, this will work as downstream code only use 31000-36000
for
> > it's idea, but it will make our life harder for upstreaming. The ordering I
> > suggest would make it easier (though it is probably much more work for you).
> 
> Right, so I haven't worked downstream in a while and no longer have access to
> the code, so no peeking for me.
> 
> If everything gets upstreamed, would it all go into ios/ or would some parts
end
> up elsewhere like in components/ ?

Everything will go into ios/ (as the resources and strings are iOS only).

> Maybe ios/ should not overlap with contents/ but overlap with chrome/ instead.
> There is a huge chunk below id 10000, and that should be enough for everything
> ios/.

Make sense since ios/chrome corresponds to chrome/ and ios/web corresponds to
content/.

[Lei Zhang, 2016-10-20 21:44:24]

On 2016/10/20 21:28:56, sdefresne wrote:
> Make sense since ios/chrome corresponds to chrome/ and ios/web corresponds to
> content/.

Ah, ok. I will map it that way then.

[sdefresne, 2016-10-20 22:46:01]

On 2016/10/20 21:44:24, Lei Zhang wrote:
> On 2016/10/20 21:28:56, sdefresne wrote:
> > Make sense since ios/chrome corresponds to chrome/ and ios/web corresponds
to
> > content/.
> 
> Ah, ok. I will map it that way then.

Thank you. Please ping me when the CL is ready to review with those changes.

[Lei Zhang, 2016-10-20 22:56:47]

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

[commit-bot: I haz the power, 2016-10-20 22:57:15]

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

[Lei Zhang, 2016-10-20 23:01:06]

On 2016/10/20 22:46:01, sdefresne wrote:
> On 2016/10/20 21:44:24, Lei Zhang wrote:
> > On 2016/10/20 21:28:56, sdefresne wrote:
> > > Make sense since ios/chrome corresponds to chrome/ and ios/web corresponds
> to
> > > content/.
> > 
> > Ah, ok. I will map it that way then.
> 
> Thank you. Please ping me when the CL is ready to review with those changes.

Ping. The diff from patch set 3 to 4 should be easy to read because I didn't
take the "organize is into net, ui, components ..." suggestion. With the new
overlap, there is plenty of space for ios/.

[commit-bot: I haz the power, 2016-10-21 00:48:06]

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

[commit-bot: I haz the power, 2016-10-21 00:48:07]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Lei Zhang, 2016-10-21 03:03:08]

https://codereview.chromium.org/2429213007/diff/60001/tools/gritsettings/reso...
File tools/gritsettings/resource_ids (right):

https://codereview.chromium.org/2429213007/diff/60001/tools/gritsettings/reso...
tools/gritsettings/resource_ids:329: "includes": [26850],
ananta: AshNativeCursorManagerTest.SetCursor is failing on Windows and I can't
figure out why. If I put this back to 4500, then it works. If I change it to
other values like 4520 or 8000, it fails. I'm not seeing a hard coded value. Any
ideas?

[sdefresne, 2016-10-21 11:45:34]

On 2016/10/20 23:01:06, Lei Zhang wrote:
> On 2016/10/20 22:46:01, sdefresne wrote:
> > On 2016/10/20 21:44:24, Lei Zhang wrote:
> > > On 2016/10/20 21:28:56, sdefresne wrote:
> > > > Make sense since ios/chrome corresponds to chrome/ and ios/web
corresponds
> > to
> > > > content/.
> > > 
> > > Ah, ok. I will map it that way then.
> > 
> > Thank you. Please ping me when the CL is ready to review with those changes.
> 
> Ping. The diff from patch set 3 to 4 should be easy to read because I didn't
> take the "organize is into net, ui, components ..." suggestion. With the new
> overlap, there is plenty of space for ios/.

lgtm

[flackr, 2016-10-21 17:36:51]

LGTM. I think we might be able to write something automated if redistributing
the available ids becomes a regular occurrence.

[Lei Zhang, 2016-10-21 18:10:44]

On 2016/10/21 17:36:51, flackr wrote:
> LGTM. I think we might be able to write something automated if redistributing
> the available ids becomes a regular occurrence.

Should be ok for now. I left spare room everywhere.

[Lei Zhang, 2016-10-21 21:15:05]

On 2016/10/21 03:03:08, Lei Zhang wrote:
>
https://codereview.chromium.org/2429213007/diff/60001/tools/gritsettings/reso...
> File tools/gritsettings/resource_ids (right):
> 
>
https://codereview.chromium.org/2429213007/diff/60001/tools/gritsettings/reso...
> tools/gritsettings/resource_ids:329: "includes": [26850],
> ananta: AshNativeCursorManagerTest.SetCursor is failing on Windows and I can't
> figure out why. If I put this back to 4500, then it works. If I change it to
> other values like 4520 or 8000, it fails. I'm not seeing a hard coded value.
Any
> ideas?

To answer my own question, I opened up ash_unittests.exe in a win32 resource
explorer program, and it still had cursors at ID 4500-4519 even though the IDs
here changed. Now to figure out why it's still using the old values...

[Lei Zhang, 2016-10-22 00:15:49]

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

[commit-bot: I haz the power, 2016-10-22 00:16:40]

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

[Lei Zhang, 2016-10-22 00:18:27]

On 2016/10/21 21:15:05, Lei Zhang wrote:
> To answer my own question, I opened up ash_unittests.exe in a win32 resource
> explorer program, and it still had cursors at ID 4500-4519 even though the IDs
> here changed. Now to figure out why it's still using the old values...

It turns out:
- the grit generated .rc files do not change
- so ninja does not know to regenerate the .res file
- so the executable still gets the old resource IDs.

In patch set 5, I changed grit to write comments with the resource ids into .rc
files, to get everything to properly rebuild to reflect the resource id changes.
Let's see if the Windows trybots go green now.

[commit-bot: I haz the power, 2016-10-22 00:20:54]

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

[commit-bot: I haz the power, 2016-10-22 00:20:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Lei Zhang, 2016-10-22 00:22:26]

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

[commit-bot: I haz the power, 2016-10-22 00:23:02]

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

[Lei Zhang, 2016-10-22 00:24:09]

Onwards to patch set 6 because r426908 landed in the meanwhile to give
chrome/app/generated_resources.grd a block of 7200 IDs. With this CL,
chrome/app/generated_resources.grd now has 9100.

[commit-bot: I haz the power, 2016-10-22 05:40:43]

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

[commit-bot: I haz the power, 2016-10-22 05:40:43]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Lei Zhang, 2016-10-24 18:49:55]

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

[commit-bot: I haz the power, 2016-10-24 18:50:45]

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, 2016-10-24 19:41:42]

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

[commit-bot: I haz the power, 2016-10-24 19:41:43]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Lei Zhang, 2016-10-24 22:17:54]

I think the purple bots are just wonky. Will attempt to land via CQ and hope the
purple-ness goes away.

[Lei Zhang, 2016-10-24 22:18:13]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-10-24 22:18:13]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org,
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2429213007/#ps120001
(title: "rebase to r427103")

[commit-bot: I haz the power, 2016-10-24 22:19:13]

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, 2016-10-25 01:56:36]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-25 02:00:42]

Description was changed from

==========
grit: Reorganize and compact resource_ids.

BUG=585301
==========

to

==========
grit: Reorganize and compact resource_ids.

BUG=585301

Committed: https://crrev.com/12af52210b397d1d76c541730d95c7a0375496c4
Cr-Commit-Position: refs/heads/master@{#427236}
==========

[commit-bot: I haz the power, 2016-10-25 02:00:43]

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