Introduce a way to define Android-only IDR values that doesn't require

Introduce a way to define Android-only IDR values that doesn't require
fake .grd entries.

BUG=538695
Committed: https://crrev.com/7fd875be7a96a672f851bad164ba23e716688dc9
Cr-Commit-Position: refs/heads/master@{#352987}

[Evan Stade, 2015-10-05 21:34:31]

estade@chromium.org changed reviewers:
+ aurimas@chromium.org, rouslan@chromium.org

[Evan Stade, 2015-10-05 21:34:32]

WDYT? I only removed one .grd entry as a proof of concept but as you can see
several others can be removed (and more after
https://codereview.chromium.org/1386723002/ lands)

[Evan Stade, 2015-10-05 21:44:55]

estade@chromium.org changed reviewers:
+ newt@chromium.org
- rouslan@chromium.org

[Evan Stade, 2015-10-05 21:44:55]

-rouslan, +newt

[newt (away), 2015-10-05 22:32:36]

https://codereview.chromium.org/1384153003/diff/20001/chrome/browser/android/...
File chrome/browser/android/resource_id.h (right):

https://codereview.chromium.org/1384153003/diff/20001/chrome/browser/android/...
chrome/browser/android/resource_id.h:5: // This file maps Chromium resource IDs
to Android resource IDs.
I'd add a newline after line 5

https://codereview.chromium.org/1384153003/diff/20001/chrome/browser/android/...
File chrome/browser/android/resource_mapper.cc (right):

https://codereview.chromium.org/1384153003/diff/20001/chrome/browser/android/...
chrome/browser/android/resource_mapper.cc:35: // Try lookup as an
Android-specific resource.
grd IDs only go up 2^16-1 (see tools/gritsettings/resource_ids). So you could
assign IDs 2^16 and higher when CREATE_RESOURCE_ID is used. That way, you
wouldn't need to use negative IDs or add this new code path.

Doing this double lookup also creates a potential bug where a "created" ID would
be silently treated as a "linked" ID if they happen to have the same value.

https://codereview.chromium.org/1384153003/diff/20001/chrome/browser/android/...
chrome/browser/android/resource_mapper.cc:53: #undef LINK_RESOURCE_ID
need #undef CREATE_RESOURCE_ID

[Evan Stade, 2015-10-06 01:12:24]

updated, I also renamed CREATE_RESOURCE_ID to DECLARE_RESOURCE_ID.

https://codereview.chromium.org/1384153003/diff/20001/chrome/browser/android/...
File chrome/browser/android/resource_id.h (right):

https://codereview.chromium.org/1384153003/diff/20001/chrome/browser/android/...
chrome/browser/android/resource_id.h:5: // This file maps Chromium resource IDs
to Android resource IDs.
On 2015/10/05 22:32:35, newt wrote:
> I'd add a newline after line 5

Done.

https://codereview.chromium.org/1384153003/diff/20001/chrome/browser/android/...
File chrome/browser/android/resource_mapper.cc (right):

https://codereview.chromium.org/1384153003/diff/20001/chrome/browser/android/...
chrome/browser/android/resource_mapper.cc:35: // Try lookup as an
Android-specific resource.
On 2015/10/05 22:32:35, newt wrote:
> grd IDs only go up 2^16-1 (see tools/gritsettings/resource_ids). So you could
> assign IDs 2^16 and higher when CREATE_RESOURCE_ID is used. That way, you
> wouldn't need to use negative IDs or add this new code path.

yea, that way is better. Thanks.

> 
> Doing this double lookup also creates a potential bug where a "created" ID
would
> be silently treated as a "linked" ID if they happen to have the same value.

Yea although I think that couldn't happen because theme resources are 7000 and
up. But yea depending on that would be regrettable.

https://codereview.chromium.org/1384153003/diff/20001/chrome/browser/android/...
chrome/browser/android/resource_mapper.cc:53: #undef LINK_RESOURCE_ID
On 2015/10/05 22:32:35, newt wrote:
> need #undef CREATE_RESOURCE_ID

Done.

[newt (away), 2015-10-06 01:19:29]

lgtm, thanks

[Evan Stade, 2015-10-06 01:20:11]

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

[commit-bot: I haz the power, 2015-10-06 01:21:19]

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

[commit-bot: I haz the power, 2015-10-06 01:23:22]

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

[commit-bot: I haz the power, 2015-10-06 01:23:23]

Dry run: 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...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Evan Stade, 2015-10-07 21:03:18]

please also review additional change:
chrome/browser/ui/android/infobars/confirm_infobar.cc

[newt (away), 2015-10-07 21:23:10]

lgtm

[Evan Stade, 2015-10-07 22:12:23]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-10-07 22:14:53]

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

[commit-bot: I haz the power, 2015-10-07 22:27:24]

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

[commit-bot: I haz the power, 2015-10-07 22:27:25]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Evan Stade, 2015-10-07 22:35:57]

estade@chromium.org changed reviewers:
+ sky@chromium.org, tedchoc@chromium.org

[Evan Stade, 2015-10-07 22:35:58]

OWNERS, please review

tedchoc:
  confirm_infobar.cc
sky:
  geolocation_infobar_delegate.cc
  protected_media_identifier_infobar_delegate.cc
  notification_permission_infobar_delegate.cc

[Ted C, 2015-10-07 23:41:30]

On 2015/10/07 22:35:58, Evan Stade wrote:
> OWNERS, please review
> 
> tedchoc:
>   confirm_infobar.cc

lgtm!

> sky:
>   geolocation_infobar_delegate.cc
>   protected_media_identifier_infobar_delegate.cc
>   notification_permission_infobar_delegate.cc

[sky, 2015-10-08 00:08:59]

LGTM - I'm surprised none of the files you asked me to review have android in
the name. I would have expected android only files to be named as such (as they
aren't in android specific directories).

[Evan Stade, 2015-10-08 00:14:10]

On 2015/10/08 00:08:59, sky wrote:
> LGTM - I'm surprised none of the files you asked me to review have android in
> the name. I would have expected android only files to be named as such (as
they
> aren't in android specific directories).

yes, that's partially my fault because I recently made them android-only without
moving them to an android directory or changing their name. (They were already
android-only in effect, but still compiling elsewhere.) Moving them to a
different directory has the undesirable consequence of proliferating the number
of directories (e.g. would need to add chrome/browser/android/notifications/),
but I guess I could change their names. Unfortunately I think you'd still have
to look for the purposes of OWNERS.

[Evan Stade, 2015-10-08 00:14:33]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-10-08 00:16:36]

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

[commit-bot: I haz the power, 2015-10-08 00:25:56]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-08 00:27:14]

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

[sky, 2015-10-08 14:22:39]

If you could change the names that would be much appreciated!

On Wed, Oct 7, 2015 at 5:14 PM,  <estade@chromium.org> wrote:
> On 2015/10/08 00:08:59, sky wrote:
>>
>> LGTM - I'm surprised none of the files you asked me to review have android
>> in
>> the name. I would have expected android only files to be named as such (as
>
> they
>>
>> aren't in android specific directories).
>
>
> yes, that's partially my fault because I recently made them android-only
> without
> moving them to an android directory or changing their name. (They were
> already
> android-only in effect, but still compiling elsewhere.) Moving them to a
> different directory has the undesirable consequence of proliferating the
> number
> of directories (e.g. would need to add
> chrome/browser/android/notifications/),
> but I guess I could change their names. Unfortunately I think you'd still
> have
> to look for the purposes of OWNERS.
>
> https://codereview.chromium.org/1384153003/

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