Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2632333002:
Add a prototype today extension behind gn flag (Closed)
|
Created:
3 years, 11 months ago by lody Modified:
3 years, 11 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, extensions-reviews_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a prototype today extension behind gn flag
As each extension requires a mobileprovision, in order not to request one
unnecessarily, this prototype re-uses the current today extension's.
BUG=none
Review-Url: https://codereview.chromium.org/2632333002
Cr-Commit-Position: refs/heads/master@{#444702}
Committed: https://chromium.googlesource.com/chromium/src/+/1a0f61231637ff47dc982aa8f6016e68053c3975
Patch Set 1 #
Total comments: 28
Patch Set 2 : arc and comments #
Total comments: 14
Patch Set 3 : renaming #
Total comments: 8
Patch Set 4 : more renaming #
Total comments: 2
Patch Set 5 : one more rename #Patch Set 6 : rebase + owner #Dependent Patchsets: Messages
Total messages: 28 (11 generated)
lod@chromium.org changed reviewers: + olivierrobin@chromium.org
Since I made a new folder you're not OWNER, but could you please review since you know most about creating extensions? Thanks.
https://codereview.chromium.org/2632333002/diff/1/ios/BUILD.gn File ios/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/1/ios/BUILD.gn#newcode48 ios/BUILD.gn:48: # dependent code has been upstreamed. I think we can remove this. Can you see with sdefresne? https://codereview.chromium.org/2632333002/diff/1/ios/BUILD.gn#newcode51 ios/BUILD.gn:51: if (!ios_enable_today_extension && ios_enable_proto_today_extension) { This will add dep to today_extension if both flags are false. https://codereview.chromium.org/2632333002/diff/1/ios/build/chrome_build.gni File ios/build/chrome_build.gni (right): https://codereview.chromium.org/2632333002/diff/1/ios/build/chrome_build.gni#... ios/build/chrome_build.gni:12: ios_enable_proto_today_extension = true Please do not submit this. https://codereview.chromium.org/2632333002/diff/1/ios/build/chrome_build.gni#... ios/build/chrome_build.gni:28: assert(!(ios_enable_today_extension && ios_enable_proto_today_extension), "Bothe today extension cannot be enabled at the same time). Then test only one or the other. https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... File ios/chrome/proto_today_extension/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... ios/chrome/proto_today_extension/BUILD.gn:17: plist_templates = [ "../today_extension/entitlements/external/today_extension.appex.entitlements" ] + ios_chrome_entitlements_additions proto_today_extension https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... ios/chrome/proto_today_extension/BUILD.gn:17: plist_templates = [ "../today_extension/entitlements/external/today_extension.appex.entitlements" ] + ios_chrome_entitlements_additions You have to use the same appex name everywhere. https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... File ios/chrome/proto_today_extension/widget_view.h (right): https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... ios/chrome/proto_today_extension/widget_view.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... ios/chrome/proto_today_extension/widget_view.h:10: @class WidgetView; remove https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... File ios/chrome/proto_today_extension/widget_view_controller.mm (right): https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... ios/chrome/proto_today_extension/widget_view_controller.mm:4: #import "ios/chrome/proto_today_extension/widget_view_controller.h" Add space before https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... ios/chrome/proto_today_extension/widget_view_controller.mm:11: @property(nonatomic) WidgetView* widgetView; Is this ARC?
drive-by (because I've been summoned by olivier) https://codereview.chromium.org/2632333002/diff/1/ios/BUILD.gn File ios/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/1/ios/BUILD.gn#newcode48 ios/BUILD.gn:48: # dependent code has been upstreamed. On 2017/01/17 10:13:05, Olivier Robin wrote: > I think we can remove this. > Can you see with sdefresne? Yes, just revert the whole file. I'll remove the dependency in a separate CL. https://codereview.chromium.org/2632333002/diff/1/ios/chrome/app/BUILD.gn File ios/chrome/app/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/1/ios/chrome/app/BUILD.gn#new... ios/chrome/app/BUILD.gn:280: if (!ios_enable_today_extension && ios_enable_proto_today_extension && I +1-ing olivierrobin earlier comment (use assert to check that only one of the two extension is enabled), then change this to: if (ios_enable_proto_today_extension && current_toolchain == default_toolchain) { ... } BTW, I would even recommend refactoring this to the following: if (current_toolchain == default_toolchain) { if (ios_enable_today_extension) { deps += [ ":today_extension_bundle" ] } if (ios_enable_proto_today_extension) { deps += [ ":proto_today_extension_bundle" ] } if (ios_enable_share_extension) { deps += [ ":share_extension_bundle" ] } } https://codereview.chromium.org/2632333002/diff/1/ios/chrome/app/BUILD.gn#new... ios/chrome/app/BUILD.gn:320: if (!ios_enable_today_extension && ios_enable_proto_today_extension && ditto, I would recommend refactoring this to if (current_toolchain == default_toolchain) { if (ios_enable_today_extension) { bundle_data("today_extension_bundle") { ... } } if (ios_enable_proto_today_extension) { bundle_data("proto_today_extension_bundle") { .. } } .. } https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... File ios/chrome/proto_today_extension/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... ios/chrome/proto_today_extension/BUILD.gn:17: plist_templates = [ "../today_extension/entitlements/external/today_extension.appex.entitlements" ] + ios_chrome_entitlements_additions On 2017/01/17 10:13:05, Olivier Robin wrote: > You have to use the same appex name everywhere. Why not just copy the file from ios/today_extension to ios/proto_today_extension/entitlements/external/proto_today_extension.appex.entitlements. Then you won't have to change BUILD.gn files when you rename your extension. https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... ios/chrome/proto_today_extension/BUILD.gn:39: It is recommended to use ARC for new code, so: configs += [ "//build/config/compiler:enable_arc" ] If this does not work, then file a bug and use a separate source_set for sources. https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... File ios/chrome/proto_today_extension/widget_view_controller.mm (right): https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... ios/chrome/proto_today_extension/widget_view_controller.mm:23: [self setWidgetView:[[WidgetView alloc] init]]; This leaks unless you're using ARC (which you are not as far as I can tell from the BUILD.gn file). BTW, you can use properties syntax here: self.widgetView = [[WidgetView alloc] init]; [self.view addSubview:self.widgetView];
Patchset #2 (id:20001) has been deleted
lod@chromium.org changed reviewers: + sdefresne@chromium.org
sdefresne, added you as reviewer since I need an owner and you already started reviewing.. thanks! :) https://codereview.chromium.org/2632333002/diff/1/ios/BUILD.gn File ios/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/1/ios/BUILD.gn#newcode48 ios/BUILD.gn:48: # dependent code has been upstreamed. On 2017/01/17 10:39:07, sdefresne wrote: > On 2017/01/17 10:13:05, Olivier Robin wrote: > > I think we can remove this. > > Can you see with sdefresne? > > Yes, just revert the whole file. I'll remove the dependency in a separate CL. Done. https://codereview.chromium.org/2632333002/diff/1/ios/BUILD.gn#newcode51 ios/BUILD.gn:51: if (!ios_enable_today_extension && ios_enable_proto_today_extension) { On 2017/01/17 10:13:05, Olivier Robin wrote: > This will add dep to today_extension if both flags are false. Acknowledged. https://codereview.chromium.org/2632333002/diff/1/ios/build/chrome_build.gni File ios/build/chrome_build.gni (right): https://codereview.chromium.org/2632333002/diff/1/ios/build/chrome_build.gni#... ios/build/chrome_build.gni:12: ios_enable_proto_today_extension = true On 2017/01/17 10:13:05, Olivier Robin wrote: > Please do not submit this. Done. https://codereview.chromium.org/2632333002/diff/1/ios/build/chrome_build.gni#... ios/build/chrome_build.gni:28: On 2017/01/17 10:13:05, Olivier Robin wrote: > assert(!(ios_enable_today_extension && ios_enable_proto_today_extension), "Bothe > today extension cannot be enabled at the same time). > Then test only one or the other. Done. https://codereview.chromium.org/2632333002/diff/1/ios/chrome/app/BUILD.gn File ios/chrome/app/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/1/ios/chrome/app/BUILD.gn#new... ios/chrome/app/BUILD.gn:280: if (!ios_enable_today_extension && ios_enable_proto_today_extension && On 2017/01/17 10:39:07, sdefresne wrote: > I +1-ing olivierrobin earlier comment (use assert to check that only one of the > two extension is enabled), then change this to: > > if (ios_enable_proto_today_extension && current_toolchain == > default_toolchain) { > ... > } > > BTW, I would even recommend refactoring this to the following: > > if (current_toolchain == default_toolchain) { > if (ios_enable_today_extension) { > deps += [ ":today_extension_bundle" ] > } > if (ios_enable_proto_today_extension) { > deps += [ ":proto_today_extension_bundle" ] > } > if (ios_enable_share_extension) { > deps += [ ":share_extension_bundle" ] > } > } > Done. https://codereview.chromium.org/2632333002/diff/1/ios/chrome/app/BUILD.gn#new... ios/chrome/app/BUILD.gn:320: if (!ios_enable_today_extension && ios_enable_proto_today_extension && On 2017/01/17 10:39:07, sdefresne wrote: > ditto, I would recommend refactoring this to > > if (current_toolchain == default_toolchain) { > if (ios_enable_today_extension) { > bundle_data("today_extension_bundle") { > ... > } > } > if (ios_enable_proto_today_extension) { > bundle_data("proto_today_extension_bundle") { > .. > } > } > .. > } Done. https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... File ios/chrome/proto_today_extension/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... ios/chrome/proto_today_extension/BUILD.gn:17: plist_templates = [ "../today_extension/entitlements/external/today_extension.appex.entitlements" ] + ios_chrome_entitlements_additions On 2017/01/17 10:39:07, sdefresne wrote: > On 2017/01/17 10:13:05, Olivier Robin wrote: > > You have to use the same appex name everywhere. > > Why not just copy the file from ios/today_extension to > ios/proto_today_extension/entitlements/external/proto_today_extension.appex.entitlements. > Then you won't have to change BUILD.gn files when you rename your extension. Not sure what you mean by "Then you won't have to change BUILD.gn files when you rename your extension." but done https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... File ios/chrome/proto_today_extension/widget_view.h (right): https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... ios/chrome/proto_today_extension/widget_view.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/17 10:13:05, Olivier Robin wrote: > 2017 Done. https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... ios/chrome/proto_today_extension/widget_view.h:10: @class WidgetView; On 2017/01/17 10:13:05, Olivier Robin wrote: > remove oops! done https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... File ios/chrome/proto_today_extension/widget_view_controller.mm (right): https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... ios/chrome/proto_today_extension/widget_view_controller.mm:4: #import "ios/chrome/proto_today_extension/widget_view_controller.h" On 2017/01/17 10:13:05, Olivier Robin wrote: > Add space before Done. https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... ios/chrome/proto_today_extension/widget_view_controller.mm:11: @property(nonatomic) WidgetView* widgetView; On 2017/01/17 10:13:05, Olivier Robin wrote: > Is this ARC? now it is! ;) https://codereview.chromium.org/2632333002/diff/1/ios/chrome/proto_today_exte... ios/chrome/proto_today_extension/widget_view_controller.mm:23: [self setWidgetView:[[WidgetView alloc] init]]; On 2017/01/17 10:39:07, sdefresne wrote: > This leaks unless you're using ARC (which you are not as far as I can tell from > the BUILD.gn file). > > BTW, you can use properties syntax here: > > self.widgetView = [[WidgetView alloc] init]; > [self.view addSubview:self.widgetView]; leak fixed. regarding using properties syntax, is that better?
https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/app/BUILD.gn File ios/chrome/app/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/app/BUILD.gn... ios/chrome/app/BUILD.gn:321: current_toolchain == default_toolchain) { remove current_toolchain test. https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/app/BUILD.gn... ios/chrome/app/BUILD.gn:330: "{{bundle_plugins_dir}}/{{source_file_part}}", Signature mobileprovision selection is based on bundle name in the PlugIns directory. The signing platform will not be able to sign your extension with the current official json. You will have either to rename this extension today_extension.appex (in that case, you need to clobber it when you change the flag value) and keep using the current official config, or create a custom json for you experimental extension.
https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/proto_today_... File ios/chrome/proto_today_extension/widget_view.mm (right): https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/proto_today_... ios/chrome/proto_today_extension/widget_view.mm:6: Please put ARC guards: #if !defined(__has_feature) || !__has_feature(objc_arc) #error "This file requires ARC support." #endif https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/proto_today_... File ios/chrome/proto_today_extension/widget_view_controller.mm (right): https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/proto_today_... ios/chrome/proto_today_extension/widget_view_controller.mm:11: @interface WidgetViewController () Please put ARC guard: #if !defined(__has_feature) || !__has_feature(objc_arc) #error "This file requires ARC support." #endif https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/proto_today_... ios/chrome/proto_today_extension/widget_view_controller.mm:24: WidgetView* widgetView = [[WidgetView alloc] init]; nit: you don't need the local variable self.widgetView = [[WidgetView alloc] init]; [self.view addSubview:self.widgetView]; Regarding property syntax vs using ivar, I think we prefer to use the property. Regarding property syntax vs method, I personally prefer property syntax (i.e. "self.foo = bar;") vs method syntax (i.e. "[self setFoo:bar];").
lgtm (maybe renaming the directory and targets to widget_extension) https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/app/BUILD.gn File ios/chrome/app/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/app/BUILD.gn... ios/chrome/app/BUILD.gn:330: "{{bundle_plugins_dir}}/{{source_file_part}}", On 2017/01/17 17:44:13, Olivier Robin wrote: > Signature mobileprovision selection is based on bundle name in the PlugIns > directory. > The signing platform will not be able to sign your extension with the current > official json. > > You will have either to rename this extension today_extension.appex (in that > case, you need to clobber it when you change the flag value) and keep using the > current official config, or create a custom json for you experimental extension. > Agreed, you need to rename the .appex today_extension.appex.
https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/app/BUILD.gn File ios/chrome/app/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/app/BUILD.gn... ios/chrome/app/BUILD.gn:330: "{{bundle_plugins_dir}}/{{source_file_part}}", On 2017/01/17 17:59:42, sdefresne wrote: > On 2017/01/17 17:44:13, Olivier Robin wrote: > > Signature mobileprovision selection is based on bundle name in the PlugIns > > directory. > > The signing platform will not be able to sign your extension with the current > > official json. > > > > You will have either to rename this extension today_extension.appex (in that > > case, you need to clobber it when you change the flag value) and keep using > the > > current official config, or create a custom json for you experimental > extension. > > > > Agreed, you need to rename the .appex today_extension.appex. Another possibility is to copy manually (in create_channel_ipa_packages.py) the prototype today_extension in the prototype.ipa. This would produce a daily build with the new extension.
https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/app/BUILD.gn File ios/chrome/app/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/app/BUILD.gn... ios/chrome/app/BUILD.gn:321: current_toolchain == default_toolchain) { On 2017/01/17 17:44:13, Olivier Robin wrote: > remove current_toolchain test. Done. https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/app/BUILD.gn... ios/chrome/app/BUILD.gn:330: "{{bundle_plugins_dir}}/{{source_file_part}}", On 2017/01/18 08:49:13, Olivier Robin wrote: > On 2017/01/17 17:59:42, sdefresne wrote: > > On 2017/01/17 17:44:13, Olivier Robin wrote: > > > Signature mobileprovision selection is based on bundle name in the PlugIns > > > directory. > > > The signing platform will not be able to sign your extension with the > current > > > official json. > > > > > > You will have either to rename this extension today_extension.appex (in that > > > case, you need to clobber it when you change the flag value) and keep using > > the > > > current official config, or create a custom json for you experimental > > extension. > > > > > > > Agreed, you need to rename the .appex today_extension.appex. > > Another possibility is to copy manually (in create_channel_ipa_packages.py) the > prototype today_extension in the prototype.ipa. > This would produce a daily build with the new extension. I went with the renaming option since I best understood it. https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/proto_today_... File ios/chrome/proto_today_extension/widget_view.mm (right): https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/proto_today_... ios/chrome/proto_today_extension/widget_view.mm:6: On 2017/01/17 17:47:42, sdefresne wrote: > Please put ARC guards: > > #if !defined(__has_feature) || !__has_feature(objc_arc) > #error "This file requires ARC support." > #endif Done. https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/proto_today_... File ios/chrome/proto_today_extension/widget_view_controller.mm (right): https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/proto_today_... ios/chrome/proto_today_extension/widget_view_controller.mm:11: @interface WidgetViewController () On 2017/01/17 17:47:42, sdefresne wrote: > Please put ARC guard: > > #if !defined(__has_feature) || !__has_feature(objc_arc) > #error "This file requires ARC support." > #endif Done. https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/proto_today_... ios/chrome/proto_today_extension/widget_view_controller.mm:24: WidgetView* widgetView = [[WidgetView alloc] init]; On 2017/01/17 17:47:42, sdefresne wrote: > nit: you don't need the local variable > > self.widgetView = [[WidgetView alloc] init]; > [self.view addSubview:self.widgetView]; > > Regarding property syntax vs using ivar, I think we prefer to use the property. > Regarding property syntax vs method, I personally prefer property syntax (i.e. > "self.foo = bar;") vs method syntax (i.e. "[self setFoo:bar];"). I do need it! The widgetView property is weak so if I init directly into it, xcode tells me it's going to get immediately dealloced. Whereas here, being in a local variable, it sticks around long enough to get retained by the addSubview call. (Thanks stepan for helping me understand this ;) )
lgtm with some comments https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/proto_today_... File ios/chrome/proto_today_extension/widget_view_controller.mm (right): https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/proto_today_... ios/chrome/proto_today_extension/widget_view_controller.mm:24: WidgetView* widgetView = [[WidgetView alloc] init]; On 2017/01/18 10:48:41, lody wrote: > On 2017/01/17 17:47:42, sdefresne wrote: > > nit: you don't need the local variable > > > > self.widgetView = [[WidgetView alloc] init]; > > [self.view addSubview:self.widgetView]; > > > > Regarding property syntax vs using ivar, I think we prefer to use the > property. > > Regarding property syntax vs method, I personally prefer property syntax (i.e. > > "self.foo = bar;") vs method syntax (i.e. "[self setFoo:bar];"). > > I do need it! The widgetView property is weak so if I init directly into it, > xcode tells me it's going to get immediately dealloced. Whereas here, being in a > local variable, it sticks around long enough to get retained by the addSubview > call. (Thanks stepan for helping me understand this ;) ) Ack. Thank you for the explanation. Can you add a comment to help future reader understand why the local variable is necessary? Something like: // Use a local variable as the property is weak and the object would // be deallocated before it can be added as a subview. https://codereview.chromium.org/2632333002/diff/60001/ios/chrome/app/BUILD.gn File ios/chrome/app/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/60001/ios/chrome/app/BUILD.gn... ios/chrome/app/BUILD.gn:329: # The output is renamed today_extension.appex so that signing in canary works and clobbering is not necessary when switching between this extension and the today extension. Please reformat your comment to fit in 80 characters per line limit. Maybe also file a crbug saying that "gn format" should reformat comments to fit in the 80 characters per line limit. Can you also add a TODO() to remove this renaming once the extension has its own provisioning profile? https://codereview.chromium.org/2632333002/diff/60001/ios/chrome/widget_exten... File ios/chrome/widget_extension/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/60001/ios/chrome/widget_exten... ios/chrome/widget_extension/BUILD.gn:39: "TODAY_EXTENSION_BUNDLE_ID=$chromium_bundle_id.TodayExtension", I would rename TODAY_EXTENSION_BUNDLE_ID to WIDGET_EXTENSION_BUNDLE_ID here (but leave the value unchanged as we only have provisioning profile for $chromium_bundle_id.TodayExtension). Please add a comment here saying that widget_extension and today_extension use the same provisioning profile during development with a TODO() to a bug to add a separate provisioning profile for the new extension (once we know that we are not going to can it). https://codereview.chromium.org/2632333002/diff/60001/ios/chrome/widget_exten... File ios/chrome/widget_extension/Info.plist (right): https://codereview.chromium.org/2632333002/diff/60001/ios/chrome/widget_exten... ios/chrome/widget_extension/Info.plist:12: <string>${IOS_BUNDLE_ID_PREFIX}.${TODAY_EXTENSION_BUNDLE_ID}</string> I would rename TODAY_EXTENSION_BUNDLE_ID to WIDGET_EXTENSION_BUNDLE_ID here too.
https://codereview.chromium.org/2632333002/diff/60001/ios/chrome/app/BUILD.gn File ios/chrome/app/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/60001/ios/chrome/app/BUILD.gn... ios/chrome/app/BUILD.gn:331: "{{bundle_plugins_dir}}/today_extension.appex", I am a little worried to have bundle name (today_extension.appex) not match CFBundleName (widget_extension) in the Info.plist. Did you actually try this solution?
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/proto_today_... File ios/chrome/proto_today_extension/widget_view_controller.mm (right): https://codereview.chromium.org/2632333002/diff/40001/ios/chrome/proto_today_... ios/chrome/proto_today_extension/widget_view_controller.mm:24: WidgetView* widgetView = [[WidgetView alloc] init]; On 2017/01/18 11:11:35, sdefresne wrote: > On 2017/01/18 10:48:41, lody wrote: > > On 2017/01/17 17:47:42, sdefresne wrote: > > > nit: you don't need the local variable > > > > > > self.widgetView = [[WidgetView alloc] init]; > > > [self.view addSubview:self.widgetView]; > > > > > > Regarding property syntax vs using ivar, I think we prefer to use the > > property. > > > Regarding property syntax vs method, I personally prefer property syntax > (i.e. > > > "self.foo = bar;") vs method syntax (i.e. "[self setFoo:bar];"). > > > > I do need it! The widgetView property is weak so if I init directly into it, > > xcode tells me it's going to get immediately dealloced. Whereas here, being in > a > > local variable, it sticks around long enough to get retained by the addSubview > > call. (Thanks stepan for helping me understand this ;) ) > > Ack. Thank you for the explanation. Can you add a comment to help future reader > understand why the local variable is necessary? Something like: > > // Use a local variable as the property is weak and the object would > // be deallocated before it can be added as a subview. The same thing is done elsewhere (I copied this from share extension) -- I am guessing it will be a common pattern in the arc future. But I will add the comment here. https://codereview.chromium.org/2632333002/diff/60001/ios/chrome/app/BUILD.gn File ios/chrome/app/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/60001/ios/chrome/app/BUILD.gn... ios/chrome/app/BUILD.gn:329: # The output is renamed today_extension.appex so that signing in canary works and clobbering is not necessary when switching between this extension and the today extension. On 2017/01/18 11:11:35, sdefresne wrote: > Please reformat your comment to fit in 80 characters per line limit. Maybe also > file a crbug saying that "gn format" should reformat comments to fit in the 80 > characters per line limit. > > Can you also add a TODO() to remove this renaming once the extension has its own > provisioning profile? Done. https://codereview.chromium.org/2632333002/diff/60001/ios/chrome/app/BUILD.gn... ios/chrome/app/BUILD.gn:331: "{{bundle_plugins_dir}}/today_extension.appex", On 2017/01/18 11:54:08, Olivier Robin wrote: > I am a little worried to have bundle name (today_extension.appex) not match > CFBundleName (widget_extension) in the Info.plist. > Did you actually try this solution? Yes I did :P https://codereview.chromium.org/2632333002/diff/60001/ios/chrome/widget_exten... File ios/chrome/widget_extension/BUILD.gn (right): https://codereview.chromium.org/2632333002/diff/60001/ios/chrome/widget_exten... ios/chrome/widget_extension/BUILD.gn:39: "TODAY_EXTENSION_BUNDLE_ID=$chromium_bundle_id.TodayExtension", On 2017/01/18 11:11:35, sdefresne wrote: > I would rename TODAY_EXTENSION_BUNDLE_ID to WIDGET_EXTENSION_BUNDLE_ID here (but > leave the value unchanged as we only have provisioning profile for > $chromium_bundle_id.TodayExtension). > > Please add a comment here saying that widget_extension and today_extension use > the same provisioning profile during development with a TODO() to a bug to add a > separate provisioning profile for the new extension (once we know that we are > not going to can it). Done. https://codereview.chromium.org/2632333002/diff/60001/ios/chrome/widget_exten... File ios/chrome/widget_extension/Info.plist (right): https://codereview.chromium.org/2632333002/diff/60001/ios/chrome/widget_exten... ios/chrome/widget_extension/Info.plist:12: <string>${IOS_BUNDLE_ID_PREFIX}.${TODAY_EXTENSION_BUNDLE_ID}</string> On 2017/01/18 11:11:35, sdefresne wrote: > I would rename TODAY_EXTENSION_BUNDLE_ID to WIDGET_EXTENSION_BUNDLE_ID here too. Done.
LGTM with comments https://codereview.chromium.org/2632333002/diff/100001/ios/chrome/widget_exte... File ios/chrome/widget_extension/Info.plist (right): https://codereview.chromium.org/2632333002/diff/100001/ios/chrome/widget_exte... ios/chrome/widget_extension/Info.plist:10: <string>proto_today_extension</string> widget_extension? https://codereview.chromium.org/2632333002/diff/100001/ios/chrome/widget_exte... ios/chrome/widget_extension/Info.plist:16: <string>proto_today_extension</string> widget_extension?
The CQ bit was checked by lod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, olivierrobin@chromium.org Link to the patchset: https://codereview.chromium.org/2632333002/#ps120001 (title: "one more rename")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for ios/chrome/app/BUILD.gn:
While running git apply --index -p1;
error: patch failed: ios/chrome/app/BUILD.gn:298
error: ios/chrome/app/BUILD.gn: patch does not apply
Patch: ios/chrome/app/BUILD.gn
Index: ios/chrome/app/BUILD.gn
diff --git a/ios/chrome/app/BUILD.gn b/ios/chrome/app/BUILD.gn
index
c94edaee7214bbb08849d60e0b9973611c6bfab1..f78e49e67755552a1a9dde8e61b3c25bd32a91f1
100644
--- a/ios/chrome/app/BUILD.gn
+++ b/ios/chrome/app/BUILD.gn
@@ -273,12 +273,16 @@ ios_app_bundle("chrome") {
bundle_deps = [ "//ios/chrome/app/resources" ]
- if (ios_enable_today_extension && current_toolchain == default_toolchain) {
- deps += [ ":today_extension_bundle" ]
- }
-
- if (ios_enable_share_extension && current_toolchain == default_toolchain) {
- deps += [ ":share_extension_bundle" ]
+ if (current_toolchain == default_toolchain) {
+ if (ios_enable_today_extension) {
+ deps += [ ":today_extension_bundle" ]
+ }
+ if (ios_enable_widget_extension) {
+ deps += [ ":widget_extension_bundle" ]
+ }
+ if (ios_enable_share_extension) {
+ deps += [ ":share_extension_bundle" ]
+ }
}
extra_substitutions = [
@@ -298,31 +302,53 @@ ios_app_bundle("chrome") {
}
}
-if (ios_enable_today_extension && current_toolchain == default_toolchain) {
- bundle_data("today_extension_bundle") {
- public_deps = [
- "//ios/chrome/today_extension",
- ]
- sources = [
- "$root_out_dir/today_extension.appex",
- ]
- outputs = [
- "{{bundle_plugins_dir}}/{{source_file_part}}",
- ]
+if (current_toolchain == default_toolchain) {
+ if (ios_enable_today_extension) {
+ bundle_data("today_extension_bundle") {
+ public_deps = [
+ "//ios/chrome/today_extension",
+ ]
+ sources = [
+ "$root_out_dir/today_extension.appex",
+ ]
+ outputs = [
+ "{{bundle_plugins_dir}}/{{source_file_part}}",
+ ]
+ }
+ }
+
+ if (ios_enable_widget_extension) {
+ bundle_data("widget_extension_bundle") {
+ public_deps = [
+ "//ios/chrome/widget_extension",
+ ]
+ sources = [
+ "$root_out_dir/widget_extension.appex",
+ ]
+
+ # The output is renamed today_extension.appex so that signing in canary
+ # works and clobbering is not necessary when switching between this
+ # extension and the today extension.
+ # TODO(crbug.com/682230) : Rename this when widget gets its own
+ # mobileprovision.
+ outputs = [
+ "{{bundle_plugins_dir}}/today_extension.appex",
+ ]
+ }
}
-}
-if (ios_enable_share_extension && current_toolchain == default_toolchain) {
- bundle_data("share_extension_bundle") {
- public_deps = [
- "//ios/chrome/share_extension",
- ]
- sources = [
- "$root_out_dir/share_extension.appex",
- ]
- outputs = [
- "{{bundle_plugins_dir}}/{{source_file_part}}",
- ]
+ if (ios_enable_share_extension) {
+ bundle_data("share_extension_bundle") {
+ public_deps = [
+ "//ios/chrome/share_extension",
+ ]
+ sources = [
+ "$root_out_dir/share_extension.appex",
+ ]
+ outputs = [
+ "{{bundle_plugins_dir}}/{{source_file_part}}",
+ ]
+ }
}
}
The CQ bit was checked by lod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from olivierrobin@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2632333002/#ps140001 (title: "rebase + owner")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484819741946970,
"parent_rev": "a0682f1bde36295a0c8f5758ab5adcb76c885b4e", "commit_rev":
"1a0f61231637ff47dc982aa8f6016e68053c3975"}
Message was sent while issue was closed.
Description was changed from ========== Add a prototype today extension behind gn flag As each extension requires a mobileprovision, in order not to request one unnecessarily, this prototype re-uses the current today extension's. BUG=none ========== to ========== Add a prototype today extension behind gn flag As each extension requires a mobileprovision, in order not to request one unnecessarily, this prototype re-uses the current today extension's. BUG=none Review-Url: https://codereview.chromium.org/2632333002 Cr-Commit-Position: refs/heads/master@{#444702} Committed: https://chromium.googlesource.com/chromium/src/+/1a0f61231637ff47dc982aa8f601... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/1a0f61231637ff47dc982aa8f601... |
