|
|
Created:
6 years, 9 months ago by Haojian Wu Modified:
6 years, 9 months ago CC:
chromium-reviews, tfarina, chromium-apps-reviews_chromium.org, Yoyo Zhou Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[App Shell] Remove chrome_100_percent.pak dependancy.
Now app::ShellMainDelegate initializes with chrome_100_percent.pak
by default. This patch is to remove this dependancy by add all
chrome-specific resources to app_shell.pak.
BUG=348484
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255830
Patch Set 1 #Patch Set 2 : address tfarina's comment #
Total comments: 2
Patch Set 3 : update #
Total comments: 2
Patch Set 4 : update #
Total comments: 3
Patch Set 5 : update #Messages
Total messages: 22 (0 generated)
Please review it. Thanks.
Could you specify in the comment what you need it for? Btw, it is very unfortunate we need it. Does ui_test_pak helps you here? +Tony in case he has a better suggestion.
https://codereview.chromium.org/176873016/diff/20001/apps/apps.gypi File apps/apps.gypi (right): https://codereview.chromium.org/176873016/diff/20001/apps/apps.gypi#newcode114 apps/apps.gypi:114: 'chrome_resources.gyp:packed_resources', Rather than depending on the target in chrome, you should figure out which pak files you need and include them here. I think you want to depend on ui/resources/ui_resources.gyp:ui_resources. https://codereview.chromium.org/176873016/diff/20001/apps/apps.gypi#newcode127 apps/apps.gypi:127: '<(grit_out_dir)/extensions_api_resources.pak', Don't you need to add the ui_resources.pak file here?
On 2014/03/03 18:30:29, tony wrote: > https://codereview.chromium.org/176873016/diff/20001/apps/apps.gypi > File apps/apps.gypi (right): > > https://codereview.chromium.org/176873016/diff/20001/apps/apps.gypi#newcode114 > apps/apps.gypi:114: 'chrome_resources.gyp:packed_resources', > Rather than depending on the target in chrome, you should figure out which pak > files you need and include them here. I think you want to depend on > ui/resources/ui_resources.gyp:ui_resources. > > https://codereview.chromium.org/176873016/diff/20001/apps/apps.gypi#newcode127 > apps/apps.gypi:127: '<(grit_out_dir)/extensions_api_resources.pak', > Don't you need to add the ui_resources.pak file here? I have updated this patch. Please review it again.
https://codereview.chromium.org/176873016/diff/40001/apps/apps.gypi File apps/apps.gypi (right): https://codereview.chromium.org/176873016/diff/40001/apps/apps.gypi#newcode130 apps/apps.gypi:130: '<(SHARED_INTERMEDIATE_DIR)/chrome/theme_resources_100_percent.pak', Does app shell really need strings from generated_resources and resources from renderer_resources? The only IDR_ values I see in src/apps is for IDR_APP_WINDOW_* values, which are in theme_resources. Ideally, we would refactor app_shell to not depend on theme_resources, but that's probably a different bug.
On 2014/03/05 18:08:44, tony wrote: > https://codereview.chromium.org/176873016/diff/40001/apps/apps.gypi > File apps/apps.gypi (right): > > https://codereview.chromium.org/176873016/diff/40001/apps/apps.gypi#newcode130 > apps/apps.gypi:130: > '<(SHARED_INTERMEDIATE_DIR)/chrome/theme_resources_100_percent.pak', > Does app shell really need strings from generated_resources and resources from > renderer_resources? The only IDR_ values I see in src/apps is for > IDR_APP_WINDOW_* values, which are in theme_resources. > > Ideally, we would refactor app_shell to not depend on theme_resources, but > that's probably a different bug. Yeah, sorting out the apps resources is crbug.com/306688 For now at least we have IDS references in //extensions and some IDR references in //apps. :-(
https://codereview.chromium.org/176873016/diff/40001/apps/apps.gypi File apps/apps.gypi (right): https://codereview.chromium.org/176873016/diff/40001/apps/apps.gypi#newcode130 apps/apps.gypi:130: '<(SHARED_INTERMEDIATE_DIR)/chrome/theme_resources_100_percent.pak', On 2014/03/05 18:08:44, tony wrote: > Does app shell really need strings from generated_resources and resources from > renderer_resources? The only IDR_ values I see in src/apps is for > IDR_APP_WINDOW_* values, which are in theme_resources. The app_window will need the IDR_APP_WINDOW_* in theme_resources. And chrome app will need the extension api binding js code in renderer_resources, IDS_EXTENSION_MANIFEST_* in generated_resources. > Ideally, we would refactor app_shell to not depend on theme_resources, but > that's probably a different bug.
LGTM but please sort things out with Tony before committing.
On 2014/03/06 04:37:16, Haojian Wu wrote: > https://codereview.chromium.org/176873016/diff/40001/apps/apps.gypi > File apps/apps.gypi (right): > > https://codereview.chromium.org/176873016/diff/40001/apps/apps.gypi#newcode130 > apps/apps.gypi:130: > '<(SHARED_INTERMEDIATE_DIR)/chrome/theme_resources_100_percent.pak', > On 2014/03/05 18:08:44, tony wrote: > > Does app shell really need strings from generated_resources and resources from > > renderer_resources? The only IDR_ values I see in src/apps is for > > IDR_APP_WINDOW_* values, which are in theme_resources. > > The app_window will need the IDR_APP_WINDOW_* in theme_resources. > And chrome app will need the extension api binding js code in > renderer_resources, IDS_EXTENSION_MANIFEST_* in generated_resources. Ok, I would probably add a TODO and file a bug to track removing these dependencies. LGTM
https://codereview.chromium.org/176873016/diff/60001/apps/apps.gypi File apps/apps.gypi (right): https://codereview.chromium.org/176873016/diff/60001/apps/apps.gypi#newcode130 apps/apps.gypi:130: # theme_resources_100_percent.pak. james@, please check whether the TODO is expected in app_shell。
LGTM https://codereview.chromium.org/176873016/diff/60001/apps/apps.gypi File apps/apps.gypi (right): https://codereview.chromium.org/176873016/diff/60001/apps/apps.gypi#newcode128 apps/apps.gypi:128: # TODO(Haojian): extra the extension/app related reousrces nit: reousrces -> resources Also, can you use your email address in TODOs? Or just put jamescook here. https://codereview.chromium.org/176873016/diff/60001/apps/apps.gypi#newcode130 apps/apps.gypi:130: # theme_resources_100_percent.pak. On 2014/03/07 11:20:21, Haojian Wu wrote: > james@, please check whether the TODO is expected in app_shell。 Yes, that seems fine. I'm not sure if the resources only exist in the en-US.pak file or all of them (I don't know how we localize .pak files) but that seems OK. We know what you mean. :-)
The CQ bit was checked by hokein.wu@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/176873016/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
The CQ bit was checked by hokein.wu@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/176873016/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by hokein.wu@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/176873016/80001
Message was sent while issue was closed.
Change committed as 255830 |