Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
254503002:
Add mime_type_handler key to extension manifests. (Closed)
|
Created:
6 years, 8 months ago by Zachary Kuznia Modified:
6 years, 7 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd mime_type_handler key to extension manifests.
This specifies the viewer that will be used to render documents that are
a supported MIME type for the extension.
BUG=366409
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266821
Patch Set 1 #
Total comments: 5
Patch Set 2 : Code review fixes #Patch Set 3 : Make the field optional #
Messages
Total messages: 27 (0 generated)
Please take a look. Thanks, -Zach
https://codereview.chromium.org/254503002/diff/1/chrome/common/extensions/mim... File chrome/common/extensions/mime_types_handler.cc (right): https://codereview.chromium.org/254503002/diff/1/chrome/common/extensions/mim... chrome/common/extensions/mime_types_handler.cc:104: &mime_types_handler)) { so this is required if mime_types is also specified? https://codereview.chromium.org/254503002/diff/1/chrome/common/extensions/mim... File chrome/common/extensions/mime_types_handler.h (right): https://codereview.chromium.org/254503002/diff/1/chrome/common/extensions/mim... chrome/common/extensions/mime_types_handler.h:38: void SetHandlerUrl(const std::string& handler_url) { should be set_handler_url
https://codereview.chromium.org/254503002/diff/1/chrome/common/extensions/mim... File chrome/common/extensions/mime_types_handler.cc (right): https://codereview.chromium.org/254503002/diff/1/chrome/common/extensions/mim... chrome/common/extensions/mime_types_handler.cc:104: &mime_types_handler)) { On 2014/04/24 14:49:48, kalman wrote: > so this is required if mime_types is also specified? Yes. I'm going to have a transition period where the event fired is different depending whether this field is specified, but eventually this field will be required. https://codereview.chromium.org/254503002/diff/1/chrome/common/extensions/mim... File chrome/common/extensions/mime_types_handler.h (right): https://codereview.chromium.org/254503002/diff/1/chrome/common/extensions/mim... chrome/common/extensions/mime_types_handler.h:38: void SetHandlerUrl(const std::string& handler_url) { On 2014/04/24 14:49:48, kalman wrote: > should be set_handler_url Done.
https://codereview.chromium.org/254503002/diff/1/chrome/common/extensions/mim... File chrome/common/extensions/mime_types_handler.cc (right): https://codereview.chromium.org/254503002/diff/1/chrome/common/extensions/mim... chrome/common/extensions/mime_types_handler.cc:104: &mime_types_handler)) { On 2014/04/24 15:55:10, Zachary Kuznia wrote: > On 2014/04/24 14:49:48, kalman wrote: > > so this is required if mime_types is also specified? > > Yes. I'm going to have a transition period where the event fired is different > depending whether this field is specified, but eventually this field will be > required. I see. In that case it would be nice if the error message mentioned that. and won't this immediately break https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... ?
On 2014/04/24 16:13:37, kalman wrote: > https://codereview.chromium.org/254503002/diff/1/chrome/common/extensions/mim... > File chrome/common/extensions/mime_types_handler.cc (right): > > https://codereview.chromium.org/254503002/diff/1/chrome/common/extensions/mim... > chrome/common/extensions/mime_types_handler.cc:104: &mime_types_handler)) { > On 2014/04/24 15:55:10, Zachary Kuznia wrote: > > On 2014/04/24 14:49:48, kalman wrote: > > > so this is required if mime_types is also specified? > > > > Yes. I'm going to have a transition period where the event fired is different > > depending whether this field is specified, but eventually this field will be > > required. > > I see. In that case it would be nice if the error message mentioned that. > > and won't this immediately break > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > ? Thanks for catching that! The field is now properly optional.
On 2014/04/24 16:13:37, kalman wrote: > https://codereview.chromium.org/254503002/diff/1/chrome/common/extensions/mim... > File chrome/common/extensions/mime_types_handler.cc (right): > > https://codereview.chromium.org/254503002/diff/1/chrome/common/extensions/mim... > chrome/common/extensions/mime_types_handler.cc:104: &mime_types_handler)) { > On 2014/04/24 15:55:10, Zachary Kuznia wrote: > > On 2014/04/24 14:49:48, kalman wrote: > > > so this is required if mime_types is also specified? > > > > Yes. I'm going to have a transition period where the event fired is different > > depending whether this field is specified, but eventually this field will be > > required. > > I see. In that case it would be nice if the error message mentioned that. > > and won't this immediately break > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > ? Thanks for catching that! The field is now properly optional.
On 2014/04/24 17:09:50, Zachary Kuznia wrote: > On 2014/04/24 16:13:37, kalman wrote: > > > https://codereview.chromium.org/254503002/diff/1/chrome/common/extensions/mim... > > File chrome/common/extensions/mime_types_handler.cc (right): > > > > > https://codereview.chromium.org/254503002/diff/1/chrome/common/extensions/mim... > > chrome/common/extensions/mime_types_handler.cc:104: &mime_types_handler)) { > > On 2014/04/24 15:55:10, Zachary Kuznia wrote: > > > On 2014/04/24 14:49:48, kalman wrote: > > > > so this is required if mime_types is also specified? > > > > > > Yes. I'm going to have a transition period where the event fired is > different > > > depending whether this field is specified, but eventually this field will be > > > required. > > > > I see. In that case it would be nice if the error message mentioned that. > > > > and won't this immediately break > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > ? > > Thanks for catching that! The field is now properly optional. Ping.
lgtm
The CQ bit was checked by zork@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zork@chromium.org/254503002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_rel
The CQ bit was checked by zork@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zork@chromium.org/254503002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by zork@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zork@chromium.org/254503002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by zork@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zork@chromium.org/254503002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by zork@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zork@chromium.org/254503002/40001
Message was sent while issue was closed.
Change committed as 266821 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
