Autofill Credit Card resources.

Move Autofill Credit Card resources out of webkit.

This moves the autofill resources from webkit to components where autofill has its code.

This came to address the comment from https://codereview.chromium.org/172473002/#msg14.

With this we eliminated the webkit dependency from components/autofill.

BUG=328150
R=isherman@chromium.org,joi@chromium.org,oshima@chromium.org
TBR=darin

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

[Ilya Sherman, 2014-02-22 01:44:33]

Super, thanks!  One question inline, otherwise LGTM.  I'd like for Evan to also
review this, because he's much more familiar than I am with how image resources
are handled in Chromium.

https://codereview.chromium.org/165673007/diff/30001/components/component_res...
File components/component_resources.grd (right):

https://codereview.chromium.org/165673007/diff/30001/components/component_res...
components/component_resources.grd:18: </grit>
Hmm, should this file reference autofill_resources.grdp somewhere?  (I'm not
actually familiar with how exactly grit works -- is this somehow inferred
instead?)

[tfarina, 2014-02-22 02:47:06]

https://codereview.chromium.org/165673007/diff/30001/components/component_res...
File components/component_resources.grd (right):

https://codereview.chromium.org/165673007/diff/30001/components/component_res...
components/component_resources.grd:18: </grit>
On 2014/02/22 01:44:33, Ilya Sherman wrote:
> Hmm, should this file reference autofill_resources.grdp somewhere?  (I'm not
> actually familiar with how exactly grit works -- is this somehow inferred
> instead?)
still playing with this. It is much harder than that. A lot of things needs to
be setup in order to get this right and working.

the one I'm facing now is:

FAILED: cd ../../chrome; python ../tools/grit/grit/format/repack.py
../out_aura/Debug/gen/repack/resources.pak
../out_aura/Debug/gen/chrome/chrome_unscaled_resources.pak
../out_aura/Debug/gen/components/component_resources.pak
../out_aura/Debug/gen/components/autofill_resources_100_percent.pak
../out_aura/Debug/gen/components/autofill_resources_200_percent.pak
../out_aura/Debug/gen/net/net_resources.pak
../out_aura/Debug/gen/ui/ui_resources/webui_resources.pak
../out_aura/Debug/gen/chrome/browser_resources.pak
../out_aura/Debug/gen/chrome/common_resources.pak
../out_aura/Debug/gen/chrome/invalidations_resources.pak
../out_aura/Debug/gen/chrome/memory_internals_resources.pak
../out_aura/Debug/gen/chrome/net_internals_resources.pak
../out_aura/Debug/gen/chrome/signin_internals_resources.pak
../out_aura/Debug/gen/chrome/sync_internals_resources.pak
../out_aura/Debug/gen/chrome/translate_internals_resources.pak
../out_aura/Debug/gen/content/browser/tracing/tracing_resources.pak
../out_aura/Debug/gen/content/content_resources.pak
../out_aura/Debug/gen/webkit/blink_resources.pak
../out_aura/Debug/gen/webkit/devtools_resources.pak
../out_aura/Debug/gen/chrome/component_extension_resources.pak
../out_aura/Debug/gen/chrome/options_resources.pak
../out_aura/Debug/gen/chrome/quota_internals_resources.pak
../out_aura/Debug/gen/chrome/sync_file_system_internals_resources.pak
../out_aura/Debug/gen/chrome/extensions_api_resources.pak
Traceback (most recent call last):
  File "../tools/grit/grit/format/repack.py", line 27, in <module>
    main(sys.argv)
  File "../tools/grit/grit/format/repack.py", line 24, in main
    grit.format.data_pack.RePack(argv[1], argv[2:])
  File "../tools/grit/grit/format/../../grit/format/data_pack.py", line 125, in
RePack
    raise exceptions.KeyError("Duplicate keys: " + str(list(duplicate_keys)))
KeyError: 'Duplicate keys: [31000, 31001, 31002, 31003, 31004, 31005, 31006]'

I tried various numbers starting from 30600...

New snapshot up if you want to take another look.

[tfarina, 2014-02-22 02:51:47]

https://codereview.chromium.org/165673007/diff/160001/components/component_re...
File components/component_resources.gyp (right):

https://codereview.chromium.org/165673007/diff/160001/components/component_re...
components/component_resources.gyp:34: 'grit_grd_file':
'autofill/resources/autofill_resources.grd',
I had to create this grd file for two reasons:

1- looks like it is not possible to put <structures> under grit-part
2- I could not have autofill_resources.grdp because the resources are either
under autofill/resources/default_100_percent or
autofill/resources/default_200_percent.

then there wasn't a way to write the path the png, because I couldn't say
autofill/resources/default_100_percent/amex.png for obvious reasons.

[Ilya Sherman, 2014-02-22 04:05:37]

I defer to Evan for the grit part of the review

[tfarina, 2014-02-22 05:07:47]

I made more changes and got components_unittests and unit_tests passing.

Of course, this was just on Linux.

Let's see if a disaster happens on the other platforms (Win, Mac, iOS, Android).

[Jói, 2014-02-24 12:25:47]

I looked only at //components/* (not //components/autofill/...) and
//tools/gritsettings.

https://codereview.chromium.org/165673007/diff/230001/components/component_re...
File components/component_resources.gyp (right):

https://codereview.chromium.org/165673007/diff/230001/components/component_re...
components/component_resources.gyp:22: 'action_name':
'generate_autofill_resources',
I'm not sure it's a good idea to add more .grd files rather than using a .grdp
file for Autofill.

Each new .grd file means an extra grit invocation (which has some per-startup
costs) and it means work on code internal to Google to make sure the resources
make it to translators.

[tfarina, 2014-02-24 16:12:04]

https://codereview.chromium.org/165673007/diff/230001/components/component_re...
File components/component_resources.gyp (right):

https://codereview.chromium.org/165673007/diff/230001/components/component_re...
components/component_resources.gyp:22: 'action_name':
'generate_autofill_resources',
On 2014/02/24 12:25:48, Jói wrote:
> I'm not sure it's a good idea to add more .grd files rather than using a .grdp
> file for Autofill.
> 
This was the first way I went but since grit complained I ended up through this
route (a separate grd file).

I made a comment about this here:
https://codereview.chromium.org/165673007/#msg3

But I will try again putting this in a grdp, may be I just made a mistake and it
will be possible to put <structures> under grit-part.

[tfarina, 2014-02-24 16:22:30]

Looking again with a fresh mind I think the mistake I did was to put
<structures> under <grit-part> and <part> under <includes>.

I should have probably written it this way:

<release seq="1">
  <structures>
    <part file="autofill/resources/autofill_resources.grdp" />
  </structures>
</release>

then in autofill_resources.grdp
<grit-part>
  <structure>
    ...
  </structure>
</grit-part>

And not have included <part file="autofill/resources/autofill_resources.grdp" />
under <includes> section. I think I misunderstood the <includes> section.

[Jói, 2014-02-24 17:08:35]

You're right, it should be written the way you are showing now. Any
type of entry (structure, include or message) should be possible to
use in a .grdp file.

Cheers,
Jói



On Mon, Feb 24, 2014 at 4:22 PM,  <tfarina@chromium.org> wrote:
> Looking again with a fresh mind I think the mistake I did was to put
> <structures> under <grit-part> and <part> under <includes>.
>
> I should have probably written it this way:
>
> <release seq="1">
>   <structures>
>     <part file="autofill/resources/autofill_resources.grdp" />
>   </structures>
> </release>
>
> then in autofill_resources.grdp
> <grit-part>
>   <structure>
>     ...
>   </structure>
> </grit-part>
>
> And not have included <part
> file="autofill/resources/autofill_resources.grdp" />
> under <includes> section. I think I misunderstood the <includes> section.
>
> https://codereview.chromium.org/165673007/

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

[Evan Stade, 2014-02-24 18:52:22]

CL needs better description, please.

https://codereview.chromium.org/165673007/diff/230001/chrome/chrome_repack_re...
File chrome/chrome_repack_resources.gypi (right):

https://codereview.chromium.org/165673007/diff/230001/chrome/chrome_repack_re...
chrome/chrome_repack_resources.gypi:10:
'<(SHARED_INTERMEDIATE_DIR)/components/autofill_resources_100_percent.pak',
this doesn't seem right, where is the 200_percent.pak line?

I see
  chrome_repack_chrome.gypi
  chrome_repack_chrome_100_percent.gypi
  chrome_repack_chrome_200_percent.gypi

and
  chrome_repack_resources.gypi (this file)

but no
  chrome_repack_resources_100_percent.gypi   
  chrome_repack_resources_200_percent.gypi

Seems like you need to create them?

[tfarina, 2014-02-24 22:29:23]

Joi,

when making it a grdp I get the following error.

Exception: The 'filename' attribute of <output> and the 'file' attribute of
<part>
  must be bare filenames, not paths.
  
Traceback (most recent call last):
  File "/home/tfarina/chromium/src/tools/gyp/pylib/gyp/input.py", line 472, in
CallLoadTargetBuildFile
    includes, depth, check, False)
  File "/home/tfarina/chromium/src/tools/gyp/pylib/gyp/input.py", line 388, in
LoadTargetBuildFile
    build_file_data, PHASE_EARLY, variables, build_file_path)
  File "/home/tfarina/chromium/src/tools/gyp/pylib/gyp/input.py", line 1237, in
ProcessVariablesAndConditionsInDict
    build_file)
  File "/home/tfarina/chromium/src/tools/gyp/pylib/gyp/input.py", line 1252, in
ProcessVariablesAndConditionsInList
    ProcessVariablesAndConditionsInDict(item, phase, variables, build_file)
  File "/home/tfarina/chromium/src/tools/gyp/pylib/gyp/input.py", line 1237, in
ProcessVariablesAndConditionsInDict
    build_file)
  File "/home/tfarina/chromium/src/tools/gyp/pylib/gyp/input.py", line 1252, in
ProcessVariablesAndConditionsInList
    ProcessVariablesAndConditionsInDict(item, phase, variables, build_file)
  File "/home/tfarina/chromium/src/tools/gyp/pylib/gyp/input.py", line 1237, in
ProcessVariablesAndConditionsInDict
    build_file)
  File "/home/tfarina/chromium/src/tools/gyp/pylib/gyp/input.py", line 1256, in
ProcessVariablesAndConditionsInList
    expanded = ExpandVariables(item, phase, variables, build_file)
  File "/home/tfarina/chromium/src/tools/gyp/pylib/gyp/input.py", line 876, in
ExpandVariables
    replacement = str(py_module.DoMain(parsed_contents[1:])).rstrip()
  File "/home/tfarina/chromium/src/tools/grit/grit_info.py", line 138, in DoMain
    options.target_platform)
  File "/home/tfarina/chromium/src/tools/grit/grit_info.py", line 68, in Inputs
    first_ids_file=ids_file, target_platform=target_platform)
  File "/home/tfarina/chromium/src/tools/grit/grit/grd_reader.py", line 186, in
Parse
    xml.sax.parse(filename_or_stream, handler)
  File "/usr/lib/python2.7/xml/sax/__init__.py", line 33, in parse
    parser.parse(source)
  File "/usr/lib/python2.7/xml/sax/expatreader.py", line 107, in parse
    xmlreader.IncrementalParser.parse(self, source)
  File "/usr/lib/python2.7/xml/sax/xmlreader.py", line 123, in parse
    self.feed(buffer)
  File "/usr/lib/python2.7/xml/sax/expatreader.py", line 207, in feed
    self._parser.Parse(data, isFinal)
  File "/usr/lib/python2.7/xml/sax/expatreader.py", line 304, in end_element
    self._cont_handler.endElement(name)
  File "/home/tfarina/chromium/src/tools/grit/grit/grd_reader.py", line 91, in
endElement
    raise exception.GotPathExpectedFilenameOnly()
GotPathExpectedFilenameOnly: The 'filename' attribute of <output> and the 'file'
attribute of <part>
  must be bare filenames, not paths.


I don't know the internals of grit, but if possible, it would be good to add
that feature to it.

[tfarina, 2014-02-24 22:34:37]

Let me think in a layout that will fit in this grdp scheme.

component_resources.grd will have to be close to default_100_percent. Examples
below.

ui/resources/ui_resources.grd
webkit/glue/resources/webkit_resources.grd

Then may be we can have the autofill resources in:

components/resources/default_100_percent/autofill/amex.png.

[tfarina, 2014-02-25 02:43:00]

I think patch set 11 satisfies all the requirements.

PTAL!

[tfarina, 2014-02-25 03:02:21]

Joi, Tony,

which error is this (from bots)?

Traceback (most recent call last):
  File "../tools/grit/grit.py", line 15, in <module>
    grit.grit_runner.Main(sys.argv[1:])
  File
"/mnt/data/b/build/slave/android/build/src/tools/grit/grit/grit_runner.py", line
268, in Main
    toolobject.Run(options, args[1:])
  File
"/mnt/data/b/build/slave/android/build/src/tools/grit/grit/tool/build.py", line
165, in Run
    self.Process()
  File
"/mnt/data/b/build/slave/android/build/src/tools/grit/grit/tool/build.py", line
285, in Process
    self.ProcessNode(self.res, output, outfile)
  File
"/mnt/data/b/build/slave/android/build/src/tools/grit/grit/tool/build.py", line
223, in ProcessNode
    formatted = formatter(node, output_node.GetLanguage(), output_dir=base_dir)
  File
"/mnt/data/b/build/slave/android/build/src/tools/grit/grit/format/data_pack.py",
line 46, in Format
    id, value = node.GetDataPackPair(lang, UTF8)
  File
"/mnt/data/b/build/slave/android/build/src/tools/grit/grit/node/structure.py",
line 206, in GetDataPackPair
    return id, self.gatherer.GetData(lang, encoding)
  File
"/mnt/data/b/build/slave/android/build/src/tools/grit/grit/gather/chrome_scaled_image.py",
line 136, in GetData
    data = _MoveSpecialChunksToFront(data)
  File
"/mnt/data/b/build/slave/android/build/src/tools/grit/grit/gather/chrome_scaled_image.py",
line 53, in _MoveSpecialChunksToFront
    for chunk in _ChunkifyPNG(data):
  File
"/mnt/data/b/build/slave/android/build/src/tools/grit/grit/gather/chrome_scaled_image.py",
line 67, in _ChunkifyPNG
    assert data.startswith(_PNG_MAGIC)

[Jói, 2014-02-25 10:32:45]

Structurally, this looks good to me.

I'm not sure what the error is that you're seeing. Looks like pkotwicz@ added
support for custom chunks, maybe this is related. Adding him and flackr@ who
have made changes to this code.

[blundell, 2014-02-25 10:49:46]

I believe that for a long time tryjobs could not handle added binary files.
Maybe that is still true, and is the reason for the error that Thiago is finding
(i.e., the error is just because the PNG files being referenced aren't actually
there)?

Thanks,

Colin

https://codereview.chromium.org/165673007/diff/450001/components/autofill.gypi
File components/autofill.gypi (right):

https://codereview.chromium.org/165673007/diff/450001/components/autofill.gyp...
components/autofill.gypi:332:
'../chrome/chrome_resources.gyp:packed_extra_resources',
What is the need for this, here and above? This seems structurally wrong.

[blundell, 2014-02-25 10:49:51]

I believe that for a long time tryjobs could not handle added binary files.
Maybe that is still true, and is the reason for the error that Thiago is finding
(i.e., the error is just because the PNG files being referenced aren't actually
there)?

Thanks,

Colin

[tfarina, 2014-02-25 13:45:31]

https://codereview.chromium.org/165673007/diff/450001/components/autofill.gypi
File components/autofill.gypi (right):

https://codereview.chromium.org/165673007/diff/450001/components/autofill.gyp...
components/autofill.gypi:332:
'../chrome/chrome_resources.gyp:packed_extra_resources',
On 2014/02/25 10:49:47, blundell wrote:
> What is the need for this, here and above? This seems structurally wrong.
It is here because component_resources.pak is packed in
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_repa...
and that rule (packed_extra_resources) is the only one that includes
chrome_repack_resources.gypi

[tfarina, 2014-02-25 19:55:26]

+Oshima, who is also familiar with resources (he is the OWNER in ui/resources/
and I believe he wrote resource_scale_factors.py).

Oshima, could you take a look here, please?

Thanks!

[flackr, 2014-02-25 19:58:23]

The commit queue and tryjob systems don't support uploading binary files. You
have to land these separately using dcommit. Typically we land a safe binaries
only CL in advance of the code that depends on it.

[oshima, 2014-02-25 20:00:26]

On 2014/02/25 19:55:26, tfarina wrote:
> +Oshima, who is also familiar with resources (he is the OWNER in ui/resources/
> and I believe he wrote resource_scale_factors.py).
> 
> Oshima, could you take a look here, please?
> 
> Thanks!

Looks like components/resources is new directory right? Could you please create
common subdirectory (assuming they're used on all platforms)?

[tfarina, 2014-02-25 20:03:58]

On 2014/02/25 19:58:23, flackr wrote:
> The commit queue and tryjob systems don't support uploading binary files. You
> have to land these separately using dcommit. Typically we land a safe binaries
> only CL in advance of the code that depends on it.

Thanks for the info Robert!

So my plan now is to make a separate CL to copy the pngs in advance. After it
lands, I will update this CL.

[tfarina, 2014-02-25 20:05:52]

On 2014/02/25 20:00:26, oshima wrote:
> On 2014/02/25 19:55:26, tfarina wrote:
> > +Oshima, who is also familiar with resources (he is the OWNER in
ui/resources/
> > and I believe he wrote resource_scale_factors.py).
> > 
> > Oshima, could you take a look here, please?
> > 
> > Thanks!
> 
> Looks like components/resources is new directory right?
Yep. We are going with this new (shared) structure.

> Could you please create common subdirectory (assuming they're used on all
platforms)?

Sure, in the separate CL (to land the pngs first) I will make sure I copy them
to components/resources/default_100_percent/common/autofill/* and
components/resources/default_200_percent/common/autofill/*

Thanks for the quick review Oshima!

[Jói, 2014-02-26 09:16:36]

Doh, I should have realized about the binary files. Thanks for
catching that Rob!

On Tue, Feb 25, 2014 at 8:05 PM,  <tfarina@chromium.org> wrote:
> On 2014/02/25 20:00:26, oshima wrote:
>>
>> On 2014/02/25 19:55:26, tfarina wrote:
>> > +Oshima, who is also familiar with resources (he is the OWNER in
>
> ui/resources/
>>
>> > and I believe he wrote resource_scale_factors.py).
>> >
>> > Oshima, could you take a look here, please?
>> >
>> > Thanks!
>
>
>> Looks like components/resources is new directory right?
>
> Yep. We are going with this new (shared) structure.
>
>
>> Could you please create common subdirectory (assuming they're used on all
>
> platforms)?
>
> Sure, in the separate CL (to land the pngs first) I will make sure I copy
> them
> to components/resources/default_100_percent/common/autofill/* and
> components/resources/default_200_percent/common/autofill/*
>
> Thanks for the quick review Oshima!
>
>
>
> https://codereview.chromium.org/165673007/
>
> --
> You received this message because you are subscribed to the Google Groups
> "browser-components-watch" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to browser-components-watch+unsubscribe@chromium.org.
> To post to this group, send email to browser-components-watch@chromium.org.
> To view this discussion on the web visit
>
https://groups.google.com/a/chromium.org/d/msgid/browser-components-watch/20c....
>
> For more options, visit
> https://groups.google.com/a/chromium.org/groups/opt_out.

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

[tfarina, 2014-02-26 23:25:25]

I have rebased it. This is ready for another look. Hopefully trybots won't
complain again.

[Jói, 2014-02-27 09:24:38]

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

https://codereview.chromium.org/165673007/diff/470001/tools/gritsettings/reso...
tools/gritsettings/resource_ids:196: "structures": [32000],
This is above the 31000 limit (see line 201) and also above the next item in the
list.

Suggest:

"includes": [30250],
"structures": [30350],

[blundell, 2014-02-27 16:18:46]

https://codereview.chromium.org/165673007/diff/470001/components/autofill.gypi
File components/autofill.gypi (right):

https://codereview.chromium.org/165673007/diff/470001/components/autofill.gyp...
components/autofill.gypi:92:
'../chrome/chrome_resources.gyp:packed_extra_resources',
Could you add a bug for eliminating this dependency and reference it here and
below?

[tfarina, 2014-02-27 16:56:58]

https://codereview.chromium.org/165673007/diff/470001/components/autofill.gypi
File components/autofill.gypi (right):

https://codereview.chromium.org/165673007/diff/470001/components/autofill.gyp...
components/autofill.gypi:92:
'../chrome/chrome_resources.gyp:packed_extra_resources',
On 2014/02/27 16:18:47, blundell wrote:
> Could you add a bug for eliminating this dependency and reference it here and
> below?
Filed the bug in crbug.com/347585. I will update the code later tonight.

[tfarina, 2014-02-27 17:01:37]

https://codereview.chromium.org/165673007/diff/470001/components/autofill.gypi
File components/autofill.gypi (right):

https://codereview.chromium.org/165673007/diff/470001/components/autofill.gyp...
components/autofill.gypi:92:
'../chrome/chrome_resources.gyp:packed_extra_resources',
Actually, I'm gonna give this another shot and see if it is really needed here.
I don't fully remember why I put this here.

I'm hoping it is not, because of the ios failure,
http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds....

[tfarina, 2014-02-28 01:29:36]

Colin, packed_extra_resources was not necessary.

At least I did:

- Removed it from autofill.gypi
- Reran gyp
- rm -rf out_aura/Debug
- ninja -C out_aura/Debug components_unittests unit_tests
- Reran the unit tests.

PTAL!

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

https://codereview.chromium.org/165673007/diff/470001/tools/gritsettings/reso...
tools/gritsettings/resource_ids:196: "structures": [32000],
On 2014/02/27 09:24:39, Jói wrote:
> This is above the 31000 limit (see line 201) and also above the next item in
the
> list.
> 
> Suggest:
> 
> "includes": [30250],
> "structures": [30350],

Done.

[tfarina, 2014-02-28 17:34:48]

Joi, Evan, ping?

[Jói, 2014-02-28 18:35:08]

components/autofill.gypi, components/component_resources.gyp,
components/resources/* and tools/gritsettings/resource_ids LGTM

[Evan Stade, 2014-02-28 18:45:48]

I still don't get why you're adding default_100_percent references without
matching default_200_percent references anywhere.

[tfarina, 2014-02-28 19:03:59]

On 2014/02/28 18:45:48, Evan Stade wrote:
> I still don't get why you're adding default_100_percent references without
> matching default_200_percent references anywhere.

Could you point out on the code this? Is the changes in component_resources.grd
you are talking about?

[Evan Stade, 2014-02-28 19:10:08]

On 2014/02/28 19:03:59, tfarina wrote:
> On 2014/02/28 18:45:48, Evan Stade wrote:
> > I still don't get why you're adding default_100_percent references without
> > matching default_200_percent references anywhere.
> 
> Could you point out on the code this? Is the changes in
component_resources.grd
> you are talking about?

yes

[Ilya Sherman, 2014-02-28 19:31:28]

https://codereview.chromium.org/165673007/diff/490001/components/component_re...
File components/component_resources.grd (left):

https://codereview.chromium.org/165673007/diff/490001/components/component_re...
components/component_resources.grd:16: </grit>
Hmm, did you mean to delete this file rather than leaving it empty?

[tfarina, 2014-02-28 19:37:28]

https://codereview.chromium.org/165673007/diff/490001/components/component_re...
File components/component_resources.grd (left):

https://codereview.chromium.org/165673007/diff/490001/components/component_re...
components/component_resources.grd:16: </grit>
On 2014/02/28 19:31:29, Ilya Sherman wrote:
> Hmm, did you mean to delete this file rather than leaving it empty?

If you go to 'View unified diff' you will see that it was deleted.

https://codereview.chromium.org/165673007/patch/490001/500001?column_width=80

It is rietveld that does not do a good job with git moves I think.

[Ilya Sherman, 2014-02-28 19:46:53]

https://codereview.chromium.org/165673007/diff/490001/components/component_re...
File components/component_resources.grd (left):

https://codereview.chromium.org/165673007/diff/490001/components/component_re...
components/component_resources.grd:16: </grit>
On 2014/02/28 19:37:29, tfarina wrote:
> On 2014/02/28 19:31:29, Ilya Sherman wrote:
> > Hmm, did you mean to delete this file rather than leaving it empty?
> 
> If you go to 'View unified diff' you will see that it was deleted.
> 
> https://codereview.chromium.org/165673007/patch/490001/500001?column_width=80
> 
> It is rietveld that does not do a good job with git moves I think.

Ah, ok :)

[Ilya Sherman, 2014-02-28 19:58:10]

https://codereview.chromium.org/165673007/diff/490001/components/resources/co...
File components/resources/component_resources.grd (right):

https://codereview.chromium.org/165673007/diff/490001/components/resources/co...
components/resources/component_resources.grd:8: <output
filename="component_resources.rc" type="rc_all" context="default_100_percent" />
So, looking at e.g. [
https://code.google.com/p/chromium/codesearch#chromium/src/ash/resources/ash_...
], it seems like it's probably appropriate to have component_resources.h have a
context of default_100_percent; but I expect that you'll need to have two
versions of component_resources.pak.  I'm not sure about component_resources.rc.

oshima@ can probably help explain exactly what this does and when it is needed. 
I'm pretty much just making inferences based on pattern-matching.

[tfarina, 2014-02-28 20:27:43]

Evan, good catch! Done.

[Evan Stade, 2014-02-28 21:46:37]

Has this been manually tested on mac or another environment that uses the 200%
resources?

I don't really feel qualified to review the grd changes, but manual testing
would go a long way towards making me feel more comfortable with this change.

[Ilya Sherman, 2014-03-01 01:29:26]

On 2014/02/28 21:46:37, Evan Stade wrote:
> Has this been manually tested on mac or another environment that uses the 200%
> resources?
> 
> I don't really feel qualified to review the grd changes, but manual testing
> would go a long way towards making me feel more comfortable with this change.

Thiago doesn't have access to a Mac nor any other environment that uses the 200%
resources.  I bet it's possible to test with a ChromeOS build on Linux somehow,
but I'm not sure how to do that.

oshima@, could you please review the *.grd* changes?  Also, if you have any tips
for testing the 100% vs. the 200% resources on a Linux box, please let us know. 
Otherwise, I think it's ok to land this, and possibly fix issues with 200%
resources in a follow-up CL, since only Dev channel users will be affected, and
the bug would just be a few blurry icons.  Evan, does that sound reasonable to
you?

[oshima, 2014-03-03 17:15:23]

Looks like this grd contains both png files and html files, am I correct? Are
these png file for C++ or HTML files/WebUI?

default_{xxx}_percent makes sense only for these png files referenced by IDR_xxx
(mostly c++, but some are done via url), and grd file for them should be
separate file (from ones for html/js resources).

https://codereview.chromium.org/165673007/diff/510001/components/resources/co...
File components/resources/component_resources.grd (right):

https://codereview.chromium.org/165673007/diff/510001/components/resources/co...
components/resources/component_resources.grd:8: <output
filename="component_resources_200_percent.pak" type="data_package"
context="default_200_percent" />
This is correct if this grd is for scaled (1x and 2x) images.

The unscaled images can be in .rc on Windows, but all scaled images has to be in
pak files because images in rc cannot be swtiched on the fly. (that's my
understanding at least).

[oshima, 2014-03-03 17:17:28]

On 2014/03/01 01:29:26, Ilya Sherman wrote:
> On 2014/02/28 21:46:37, Evan Stade wrote:
> > Has this been manually tested on mac or another environment that uses the
200%
> > resources?
> > 
> > I don't really feel qualified to review the grd changes, but manual testing
> > would go a long way towards making me feel more comfortable with this
change.
> 
> Thiago doesn't have access to a Mac nor any other environment that uses the
200%
> resources.  I bet it's possible to test with a ChromeOS build on Linux
somehow,
> but I'm not sure how to do that.
> 
> oshima@, could you please review the *.grd* changes?  Also, if you have any
tips
> for testing the 100% vs. the 200% resources on a Linux box, please let us
know. 
> Otherwise, I think it's ok to land this, and possibly fix issues with 200%
> resources in a follow-up CL, since only Dev channel users will be affected,
and
> the bug would just be a few blurry icons.  Evan, does that sound reasonable to
> you?

To test 2x assets on chrome for chromeos on desktop (such as Uoobuntu), build
chrome for chromeos (chromeos=1)
and run with --force-device-scale-factor=2. Nico should know how to test on mac.

[Ilya Sherman, 2014-03-04 02:02:11]

On 2014/03/03 17:15:23, oshima wrote:
> Looks like this grd contains both png files and html files, am I correct? Are
> these png file for C++ or HTML files/WebUI?
> 
> default_{xxx}_percent makes sense only for these png files referenced by
IDR_xxx
> (mostly c++, but some are done via url), and grd file for them should be
> separate file (from ones for html/js resources).

These png files are used both in native (C++-based) UIs and in Web
(HTML/CSS/JS-based) UIs.  I'm assuming that default_{xxx} still makes sense in
that case -- is that correct?

[oshima, 2014-03-04 10:00:50]

On 2014/03/04 02:02:11, Ilya Sherman wrote:
> On 2014/03/03 17:15:23, oshima wrote:
> > Looks like this grd contains both png files and html files, am I correct?
Are
> > these png file for C++ or HTML files/WebUI?
> > 
> > default_{xxx}_percent makes sense only for these png files referenced by
> IDR_xxx
> > (mostly c++, but some are done via url), and grd file for them should be
> > separate file (from ones for html/js resources).
> 
> These png files are used both in native (C++-based) UIs and in Web
> (HTML/CSS/JS-based) UIs.  I'm assuming that default_{xxx} still makes sense in
> that case -- is that correct?

Yes, if they are really used in both, although my guess is not all of them are
used in both.
If they're mostly shared, then it's not a big deal,

See http://www.chromium.org/developers/design-documents/high-dpi-resources for
more info.

[Ilya Sherman, 2014-03-05 08:16:10]

On 2014/03/04 10:00:50, oshima wrote:
> On 2014/03/04 02:02:11, Ilya Sherman wrote:
> > On 2014/03/03 17:15:23, oshima wrote:
> > > Looks like this grd contains both png files and html files, am I correct?
> Are
> > > these png file for C++ or HTML files/WebUI?
> > > 
> > > default_{xxx}_percent makes sense only for these png files referenced by
> > IDR_xxx
> > > (mostly c++, but some are done via url), and grd file for them should be
> > > separate file (from ones for html/js resources).
> > 
> > These png files are used both in native (C++-based) UIs and in Web
> > (HTML/CSS/JS-based) UIs.  I'm assuming that default_{xxx} still makes sense
in
> > that case -- is that correct?
> 
> Yes, if they are really used in both, although my guess is not all of them are
> used in both.
> If they're mostly shared, then it's not a big deal,
> 
> See http://www.chromium.org/developers/design-documents/high-dpi-resources for
> more info.

Hmm.  The new resources being added in this CL -- those for Autofill -- are used
in both.  I'm not sure about the pre-existing resources.

[oshima, 2014-03-06 00:23:04]

could you please rename

autofill_resources.grdp

to

autofill_scaled_resources.grdp

? then lgtm

[Ilya Sherman, 2014-03-06 00:30:11]

Could you please also manually verify that running a ChromeOS build with forced
scaling set to 2 (as per oshima@'s instructions) results in the correct
resources being used for the following two sets of STR?

STR 1:
1. Navigate to chrome://settings/autofill
2. Add a new credit card with number "4111111111111111"
3. Save that card.  It should have a 'Visa' icon.

STR 2:
1. Start with STR 1, so you have that card saved.
2. Navigate to [ https://googledrive.com/host/0B28BnxIvH5DuWGc3Mm5sZE9DekE/ ]
3. Add something to your basket.
4. Click "Checkout".
5. Find a way to show the credit card icon in the dialog, e.g. by starting to
type "4111..." into the credit card number field.  The 'Visa' icon should again
be displayed, and should look crisp.

Thanks again for taking on this migration, Thiago! :)

[oshima, 2014-03-06 00:41:52]

just one question (still lgtm if you make the aforementioned change)

https://codereview.chromium.org/165673007/diff/560001/components/resources/co...
File components/resources/component_resources.grd (left):

https://codereview.chromium.org/165673007/diff/560001/components/resources/co...
components/resources/component_resources.grd:8: <output
filename="component_resources.rc" type="rc_all" />
I assume this was not used regardless of this change, correct?

[tfarina, 2014-03-06 00:58:06]

On 2014/03/06 00:23:04, oshima wrote:
> could you please rename
> 
> autofill_resources.grdp
> 
> to
> 
> autofill_scaled_resources.grdp
> 
> ? then lgtm
Done.

I made some other changes, if you want to take another look...

https://codereview.chromium.org/165673007/diff/560001/components/resources/co...
File components/resources/component_resources.grd (left):

https://codereview.chromium.org/165673007/diff/560001/components/resources/co...
components/resources/component_resources.grd:8: <output
filename="component_resources.rc" type="rc_all" />
On 2014/03/06 00:41:53, oshima wrote:
> I assume this was not used regardless of this change, correct?

that is correct.

[tfarina, 2014-03-06 14:07:17]

Ilya, thanks for the detailed test instructions.

I followed them this morning (after leaving chromeos config building during the
night) and attached the screenshots that I took to the bug.

[Ilya Sherman, 2014-03-07 00:12:27]

On 2014/03/06 14:07:17, tfarina wrote:
> Ilya, thanks for the detailed test instructions.
> 
> I followed them this morning (after leaving chromeos config building during
the
> night) and attached the screenshots that I took to the bug.

Thanks!  Looking at the screenshots, the icon is rendered sharply in the
requestAutocomplete dialog (bottom screenshot), but appears blurry in the
settings page.  It should appear crisp in both.  Is this a regression from your
change, or a pre-existing bug in the settings page implementation?

[Ilya Sherman, 2014-03-07 00:20:38]

On 2014/03/07 00:12:27, Ilya Sherman wrote:
> On 2014/03/06 14:07:17, tfarina wrote:
> > Ilya, thanks for the detailed test instructions.
> > 
> > I followed them this morning (after leaving chromeos config building during
> the
> > night) and attached the screenshots that I took to the bug.
> 
> Thanks!  Looking at the screenshots, the icon is rendered sharply in the
> requestAutocomplete dialog (bottom screenshot), but appears blurry in the
> settings page.  It should appear crisp in both.  Is this a regression from
your
> change, or a pre-existing bug in the settings page implementation?

Just tested locally, and looks like it's a pre-existing bug; filed [
https://code.google.com/p/chromium/issues/detail?id=350186 ].  So, LGTM for the
changes in this CL.  Thanks, Thiago!

[tfarina, 2014-03-07 01:04:49]

On 2014/03/07 00:20:38, Ilya Sherman wrote:
> On 2014/03/07 00:12:27, Ilya Sherman wrote:
> > On 2014/03/06 14:07:17, tfarina wrote:
> > > Ilya, thanks for the detailed test instructions.
> > > 
> > > I followed them this morning (after leaving chromeos config building
during
> > the
> > > night) and attached the screenshots that I took to the bug.
> > 
> > Thanks!  Looking at the screenshots, the icon is rendered sharply in the
> > requestAutocomplete dialog (bottom screenshot), but appears blurry in the
> > settings page.  It should appear crisp in both.  Is this a regression from
> your
> > change, or a pre-existing bug in the settings page implementation?
> 
> Just tested locally, and looks like it's a pre-existing bug; filed [
> https://code.google.com/p/chromium/issues/detail?id=350186 ].  So, LGTM for
the
> changes in this CL.  Thanks, Thiago!

Tested on master branch too. I posted a screenshot on the bug. Looks like a
pre-existing bug.

Thanks for going with me through this Ilya!

I'm going to move forward now that I think everyone is fine with current shape
of this CL. TBRing Darin for the the rest of the changes spamming chrome,
content and webkit.

[tfarina, 2014-03-07 01:05:44]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2014-03-07 01:10:50]

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

[commit-bot: I haz the power, 2014-03-07 01:52:46]

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

[commit-bot: I haz the power, 2014-03-07 01:52:48]

Try jobs failed on following builders: mac_chromium_rel

[tfarina, 2014-03-07 03:04:16]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2014-03-07 03:09:30]

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

[commit-bot: I haz the power, 2014-03-07 05:30:54]

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

[commit-bot: I haz the power, 2014-03-07 05:30:56]

Try jobs failed on following builders: mac_chromium_rel

[Ilya Sherman, 2014-03-07 05:33:13]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-03-07 05:36:08]

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

[commit-bot: I haz the power, 2014-03-07 12:07:23]

Change committed as 255594

[blundell, 2014-03-07 12:10:58]

https://codereview.chromium.org/165673007/diff/600001/components/autofill/cor...
File components/autofill/core/browser/credit_card.cc (right):

https://codereview.chromium.org/165673007/diff/600001/components/autofill/cor...
components/autofill/core/browser/credit_card.cc:167: // TODO(blundell): Either
move these resources out of webkit_resources or
Hi Thiago,

Sorry I hadn't noticed this before: Would you be up for doing a follow-up CL
removing all of the ifdefing in //components/autofill/core/browser around
crbug.com/328150 (i.e., all the ifdefing that references that bug in comments)?

[tfarina, 2014-03-07 14:37:41]

https://codereview.chromium.org/165673007/diff/600001/components/autofill/cor...
File components/autofill/core/browser/credit_card.cc (right):

https://codereview.chromium.org/165673007/diff/600001/components/autofill/cor...
components/autofill/core/browser/credit_card.cc:167: // TODO(blundell): Either
move these resources out of webkit_resources or
On 2014/03/07 12:10:59, blundell wrote:
> Hi Thiago,
> 
> Sorry I hadn't noticed this before: Would you be up for doing a follow-up CL
> removing all of the ifdefing in //components/autofill/core/browser around
> crbug.com/328150 (i.e., all the ifdefing that references that bug in
comments)?
Yes, but I will have to figure out why it has failed when I actually removed it
in the previous patch set.

[blundell1, 2014-03-07 15:01:09]

Please feel free to loop me in for help.


On Fri, Mar 7, 2014 at 3:37 PM, <tfarina@chromium.org> wrote:

>
> https://codereview.chromium.org/165673007/diff/600001/
> components/autofill/core/browser/credit_card.cc
> File components/autofill/core/browser/credit_card.cc (right):
>
> https://codereview.chromium.org/165673007/diff/600001/
> components/autofill/core/browser/credit_card.cc#newcode167
> components/autofill/core/browser/credit_card.cc:167: // TODO(blundell):
> Either move these resources out of webkit_resources or
> On 2014/03/07 12:10:59, blundell wrote:
>
>> Hi Thiago,
>>
>
>  Sorry I hadn't noticed this before: Would you be up for doing a
>>
> follow-up CL
>
>> removing all of the ifdefing in //components/autofill/core/browser
>>
> around
>
>> crbug.com/328150 (i.e., all the ifdefing that references that bug in
>>
> comments)?
> Yes, but I will have to figure out why it has failed when I actually
> removed it in the previous patch set.
>
> https://codereview.chromium.org/165673007/
>

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