Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
137833002:
Fix an out-of-date issue in chrome.webNavigation document. (Closed)
|
Created:
6 years, 11 months ago by Haojian Wu Modified:
6 years, 10 months ago Reviewers:
jochen (gone - plz use gerrit), not at google - send to devlin, Roger Tawa OOO till Jul 10th, Haojian Wu CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFix an out-of-date issue in chrome.webNavigation document.
The transitionType "start_page" is out of date, replace it with
"auto_toplevel"
BUG=333878
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247663
Patch Set 1 #
Total comments: 2
Patch Set 2 : reset "start_page" #
Total comments: 2
Patch Set 3 : update #Patch Set 4 : fix code style. #
Total comments: 4
Patch Set 5 : update doc #
Total comments: 2
Messages
Total messages: 23 (0 generated)
Please review it. Thanks.
I'm not the right person to review this. +kalman@
nor am I.
+rogerta who changed this I'm worried that this will break existing extensions. I think it's better to map AUTO_TOPLEVEL to "page_start". The webnavigation API is the only consumer of this method anyway
lgtm Note that I simply replaced the old name with the new name (sorry that I missed this instance). The functionality stayed the same. Wrt breaking existing extensions, I don't know how real that possibility it. Jochen has a good point, should check with the extension folks on that.
https://codereview.chromium.org/137833002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/web_navigation.json (right): https://codereview.chromium.org/137833002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/web_navigation.json:152: "transitionType": {"type": "string", "enum": ["link", "typed", "auto_bookmark", "auto_subframe", "manual_subframe", "generated", "auto_toplevel", "form_submit", "reload", "keyword", "keyword_generated"], "description": "Cause of the navigation. The same transition types as defined in the history API are used."}, yeah we can't just remove "start_page" for backwards compatibility. We could however go from ... "generated", "start_page", "form_submit", ... to ... "generated", {"name": "start_page", "nodoc": true} "auto_toplevel", "form_submit", ... and see how that turns out. Then like Jochen says maintain compatibility by mapping from start_page to auto_toplevel somewhere.
https://codereview.chromium.org/137833002/diff/1/chrome/common/extensions/api... File chrome/common/extensions/api/web_navigation.json (right): https://codereview.chromium.org/137833002/diff/1/chrome/common/extensions/api... chrome/common/extensions/api/web_navigation.json:152: "transitionType": {"type": "string", "enum": ["link", "typed", "auto_bookmark", "auto_subframe", "manual_subframe", "generated", "auto_toplevel", "form_submit", "reload", "keyword", "keyword_generated"], "description": "Cause of the navigation. The same transition types as defined in the history API are used."}, On 2014/01/17 22:16:02, kalman wrote: > yeah we can't just remove "start_page" for backwards compatibility. We could > however go from > > ... > "generated", > "start_page", > "form_submit", > ... > > to > > ... > "generated", > {"name": "start_page", "nodoc": true} > "auto_toplevel", > "form_submit", > ... > > > and see how that turns out. Then like Jochen says maintain compatibility by > mapping from start_page to auto_toplevel somewhere. After looking through the code, I think here is just a document issue(The doc description is not the same with the actual result). From Roger@'s words and his replaced CL(https://codereview.chromium.org/10897034/), he just replaced START_PAGE with AUTO_TOPLEVEL, keeping same functionality. So the returned 'transitionType' is 'auto_toplevel' instead of 'start_page' now(see https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...).
On 2014/01/18 04:11:35, Haojian Wu wrote: > https://codereview.chromium.org/137833002/diff/1/chrome/common/extensions/api... > File chrome/common/extensions/api/web_navigation.json (right): > > https://codereview.chromium.org/137833002/diff/1/chrome/common/extensions/api... > chrome/common/extensions/api/web_navigation.json:152: "transitionType": {"type": > "string", "enum": ["link", "typed", "auto_bookmark", "auto_subframe", > "manual_subframe", "generated", "auto_toplevel", "form_submit", "reload", > "keyword", "keyword_generated"], "description": "Cause of the navigation. The > same transition types as defined in the history API are used."}, > On 2014/01/17 22:16:02, kalman wrote: > > yeah we can't just remove "start_page" for backwards compatibility. We could > > however go from > > > > ... > > "generated", > > "start_page", > > "form_submit", > > ... > > > > to > > > > ... > > "generated", > > {"name": "start_page", "nodoc": true} > > "auto_toplevel", > > "form_submit", > > ... > > > > > > and see how that turns out. Then like Jochen says maintain compatibility by > > mapping from start_page to auto_toplevel somewhere. > > After looking through the code, I think here is just a document issue(The doc > description is not the same with the actual result). > > From Roger@'s words and his replaced > CL(https://codereview.chromium.org/10897034/), he just replaced START_PAGE with > AUTO_TOPLEVEL, keeping same functionality. So the returned 'transitionType' is > 'auto_toplevel' instead of 'start_page' now(see > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...). Yes, but that's wrong. It used to return start_page. start_page is what is documented in the API. We should update the code to return start_page again.
On 2014/01/21 09:00:37, jochen wrote: > On 2014/01/18 04:11:35, Haojian Wu wrote: > > > https://codereview.chromium.org/137833002/diff/1/chrome/common/extensions/api... > > File chrome/common/extensions/api/web_navigation.json (right): > > > > > https://codereview.chromium.org/137833002/diff/1/chrome/common/extensions/api... > > chrome/common/extensions/api/web_navigation.json:152: "transitionType": > {"type": > > "string", "enum": ["link", "typed", "auto_bookmark", "auto_subframe", > > "manual_subframe", "generated", "auto_toplevel", "form_submit", "reload", > > "keyword", "keyword_generated"], "description": "Cause of the navigation. The > > same transition types as defined in the history API are used."}, > > On 2014/01/17 22:16:02, kalman wrote: > > > yeah we can't just remove "start_page" for backwards compatibility. We could > > > however go from > > > > > > ... > > > "generated", > > > "start_page", > > > "form_submit", > > > ... > > > > > > to > > > > > > ... > > > "generated", > > > {"name": "start_page", "nodoc": true} > > > "auto_toplevel", > > > "form_submit", > > > ... > > > > > > > > > and see how that turns out. Then like Jochen says maintain compatibility by > > > mapping from start_page to auto_toplevel somewhere. > > > > After looking through the code, I think here is just a document issue(The doc > > description is not the same with the actual result). > > > > From Roger@'s words and his replaced > > CL(https://codereview.chromium.org/10897034/), he just replaced START_PAGE > with > > AUTO_TOPLEVEL, keeping same functionality. So the returned 'transitionType' is > > 'auto_toplevel' instead of 'start_page' now(see > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...). > > Yes, but that's wrong. It used to return start_page. start_page is what is > documented in the API. We should update the code to return start_page again. What Jochen said, unless there's an important reason for exposing that rename to extensions. In that case do what I suggested and support both the old and the new, with the old mapping to the new. However just supporting *only* the old sounds simpler.
On 2014/01/21 18:50:10, kalman wrote: > On 2014/01/21 09:00:37, jochen wrote: > > On 2014/01/18 04:11:35, Haojian Wu wrote: > > > > > > https://codereview.chromium.org/137833002/diff/1/chrome/common/extensions/api... > > > File chrome/common/extensions/api/web_navigation.json (right): > > > > > > > > > https://codereview.chromium.org/137833002/diff/1/chrome/common/extensions/api... > > > chrome/common/extensions/api/web_navigation.json:152: "transitionType": > > {"type": > > > "string", "enum": ["link", "typed", "auto_bookmark", "auto_subframe", > > > "manual_subframe", "generated", "auto_toplevel", "form_submit", "reload", > > > "keyword", "keyword_generated"], "description": "Cause of the navigation. > The > > > same transition types as defined in the history API are used."}, > > > On 2014/01/17 22:16:02, kalman wrote: > > > > yeah we can't just remove "start_page" for backwards compatibility. We > could > > > > however go from > > > > > > > > ... > > > > "generated", > > > > "start_page", > > > > "form_submit", > > > > ... > > > > > > > > to > > > > > > > > ... > > > > "generated", > > > > {"name": "start_page", "nodoc": true} > > > > "auto_toplevel", > > > > "form_submit", > > > > ... > > > > > > > > > > > > and see how that turns out. Then like Jochen says maintain compatibility > by > > > > mapping from start_page to auto_toplevel somewhere. > > > > > > After looking through the code, I think here is just a document issue(The > doc > > > description is not the same with the actual result). > > > > > > From Roger@'s words and his replaced > > > CL(https://codereview.chromium.org/10897034/), he just replaced START_PAGE > > with > > > AUTO_TOPLEVEL, keeping same functionality. So the returned 'transitionType' > is > > > 'auto_toplevel' instead of 'start_page' now(see > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...). > > > > Yes, but that's wrong. It used to return start_page. start_page is what is > > documented in the API. We should update the code to return start_page again. > > What Jochen said, unless there's an important reason for exposing that rename to > extensions. In that case do what I suggested and support both the old and the > new, with the old mapping to the new. However just supporting *only* the old > sounds simpler. Done. Please see patch set 2. I update the code, just return "start_page" and also update the doc.
lgtm https://codereview.chromium.org/137833002/diff/130001/chrome/browser/extensio... File chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc (right): https://codereview.chromium.org/137833002/diff/130001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:106: if (transition_type_string == "auto_toplevel") nit transition_type == PAGE_TRANSITION_TYPE_AUTO_TOPLEVEL
https://codereview.chromium.org/137833002/diff/130001/chrome/browser/extensio... File chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc (right): https://codereview.chromium.org/137833002/diff/130001/chrome/browser/extensio... chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc:106: if (transition_type_string == "auto_toplevel") On 2014/01/22 11:30:02, jochen wrote: > nit transition_type == PAGE_TRANSITION_TYPE_AUTO_TOPLEVEL Done.
https://codereview.chromium.org/137833002/diff/200001/chrome/common/extension... File chrome/common/extensions/api/web_navigation.json (right): https://codereview.chromium.org/137833002/diff/200001/chrome/common/extension... chrome/common/extensions/api/web_navigation.json:152: "transitionType": {"type": "string", "enum": ["link", "typed", "auto_bookmark", "auto_subframe", "manual_subframe", "generated", "start_page", "form_submit", "reload", "keyword", "keyword_generated"], "description": "Cause of the navigation. The same transition types as defined in the history API are used. For backward compatibility, \"start_page\" is the sames as \"auto_toplevel\" in the history API."}, why are all these comments necessary? auto_toplevel isn't exposed to the extension in any way.
https://codereview.chromium.org/137833002/diff/200001/chrome/common/extension... File chrome/common/extensions/api/web_navigation.json (right): https://codereview.chromium.org/137833002/diff/200001/chrome/common/extension... chrome/common/extensions/api/web_navigation.json:152: "transitionType": {"type": "string", "enum": ["link", "typed", "auto_bookmark", "auto_subframe", "manual_subframe", "generated", "start_page", "form_submit", "reload", "keyword", "keyword_generated"], "description": "Cause of the navigation. The same transition types as defined in the history API are used. For backward compatibility, \"start_page\" is the sames as \"auto_toplevel\" in the history API."}, On 2014/01/22 15:23:50, kalman wrote: > why are all these comments necessary? auto_toplevel isn't exposed to the > extension in any way. But as the document says "The same transition types as defined in the history API are used.", in history API doc "auto_toplevel" is exposed to extensions. see(https://developer.chrome.com/extensions/history.html)
https://codereview.chromium.org/137833002/diff/200001/chrome/common/extension... File chrome/common/extensions/api/web_navigation.json (right): https://codereview.chromium.org/137833002/diff/200001/chrome/common/extension... chrome/common/extensions/api/web_navigation.json:152: "transitionType": {"type": "string", "enum": ["link", "typed", "auto_bookmark", "auto_subframe", "manual_subframe", "generated", "start_page", "form_submit", "reload", "keyword", "keyword_generated"], "description": "Cause of the navigation. The same transition types as defined in the history API are used. For backward compatibility, \"start_page\" is the sames as \"auto_toplevel\" in the history API."}, On 2014/01/22 15:59:24, Haojian Wu wrote: > On 2014/01/22 15:23:50, kalman wrote: > > why are all these comments necessary? auto_toplevel isn't exposed to the > > extension in any way. > > But as the document says "The same transition types as defined in the history > API are used.", in history API doc "auto_toplevel" is exposed to extensions. > see(https://developer.chrome.com/extensions/history.html) Ugh. Interesting. Looks like this was changed in https://chromiumcodereview.appspot.com/10897034 in the exact same manner as this CL is trying to change; except that CL was not reviewed by an extensions owner. Damn... well, that's a shame. Ok, anyway, thanks for pointing that out. We should actually make those enums point at the same thing but it's not worth doing now (nor probably... ever, given they've now diverged). In that case I have a suggestion re wording; it should be "same as" not "sames as", but I would go further: "These are the same transition types as defined in the $ref:[history.VisitedItem History API] except with <code>\"start_page\"</code> in place of <code>\"auto_toplevel\"</code> (for backwards compatibility)." (for all such comments)
https://codereview.chromium.org/137833002/diff/200001/chrome/common/extension... File chrome/common/extensions/api/web_navigation.json (right): https://codereview.chromium.org/137833002/diff/200001/chrome/common/extension... chrome/common/extensions/api/web_navigation.json:152: "transitionType": {"type": "string", "enum": ["link", "typed", "auto_bookmark", "auto_subframe", "manual_subframe", "generated", "start_page", "form_submit", "reload", "keyword", "keyword_generated"], "description": "Cause of the navigation. The same transition types as defined in the history API are used. For backward compatibility, \"start_page\" is the sames as \"auto_toplevel\" in the history API."}, On 2014/01/22 16:06:35, kalman wrote: > On 2014/01/22 15:59:24, Haojian Wu wrote: > > On 2014/01/22 15:23:50, kalman wrote: > > > why are all these comments necessary? auto_toplevel isn't exposed to the > > > extension in any way. > > > > But as the document says "The same transition types as defined in the history > > API are used.", in history API doc "auto_toplevel" is exposed to extensions. > > see(https://developer.chrome.com/extensions/history.html) > > Ugh. Interesting. Looks like this was changed in > https://chromiumcodereview.appspot.com/10897034 in the exact same manner as this > CL is trying to change; except that CL was not reviewed by an extensions owner. > Damn... well, that's a shame. > > Ok, anyway, thanks for pointing that out. We should actually make those enums > point at the same thing but it's not worth doing now (nor probably... ever, > given they've now diverged). In that case I have a suggestion re wording; it > should be "same as" not "sames as", but I would go further: > > "These are the same transition types as defined in the $ref:[history.VisitedItem > History API] except with <code>\"start_page\"</code> in place of > <code>\"auto_toplevel\"</code> (for backwards compatibility)." > > (for all such comments) Done.
On 2014/01/23 06:44:27, Haojian Wu wrote: > https://codereview.chromium.org/137833002/diff/200001/chrome/common/extension... > File chrome/common/extensions/api/web_navigation.json (right): > > https://codereview.chromium.org/137833002/diff/200001/chrome/common/extension... > chrome/common/extensions/api/web_navigation.json:152: "transitionType": {"type": > "string", "enum": ["link", "typed", "auto_bookmark", "auto_subframe", > "manual_subframe", "generated", "start_page", "form_submit", "reload", > "keyword", "keyword_generated"], "description": "Cause of the navigation. The > same transition types as defined in the history API are used. For backward > compatibility, \"start_page\" is the sames as \"auto_toplevel\" in the history > API."}, > On 2014/01/22 16:06:35, kalman wrote: > > On 2014/01/22 15:59:24, Haojian Wu wrote: > > > On 2014/01/22 15:23:50, kalman wrote: > > > > why are all these comments necessary? auto_toplevel isn't exposed to the > > > > extension in any way. > > > > > > But as the document says "The same transition types as defined in the > history > > > API are used.", in history API doc "auto_toplevel" is exposed to extensions. > > > see(https://developer.chrome.com/extensions/history.html) > > > > Ugh. Interesting. Looks like this was changed in > > https://chromiumcodereview.appspot.com/10897034 in the exact same manner as > this > > CL is trying to change; except that CL was not reviewed by an extensions > owner. > > Damn... well, that's a shame. > > > > Ok, anyway, thanks for pointing that out. We should actually make those enums > > point at the same thing but it's not worth doing now (nor probably... ever, > > given they've now diverged). In that case I have a suggestion re wording; it > > should be "same as" not "sames as", but I would go further: > > > > "These are the same transition types as defined in the > $ref:[history.VisitedItem > > History API] except with <code>\"start_page\"</code> in place of > > <code>\"auto_toplevel\"</code> (for backwards compatibility)." > > > > (for all such comments) > > Done. ping kalman@.
https://codereview.chromium.org/137833002/diff/270001/chrome/common/extension... File chrome/common/extensions/api/web_navigation.json (right): https://codereview.chromium.org/137833002/diff/270001/chrome/common/extension... chrome/common/extensions/api/web_navigation.json:152: "transitionType": {"type": "string", "enum": ["link", "typed", "auto_bookmark", "auto_subframe", "manual_subframe", "generated", "start_page", "form_submit", "reload", "keyword", "keyword_generated"], "description": "Cause of the navigation. The same transition types as defined in the history API are used. These are the same transition types as defined in the <a href=\"history.html#transition_types\">history API</a> except with <code>\"start_page\"</code> in place of <code>\"auto_toplevel\"</code> (for backwards compatibility)."}, I thought I made this comment before actually; could you pull this transition type data out into a type and then for each of these use $ref: TransitionType as the types. You may want to annotate that TransitionType type with "inline_doc": true (I think that's it) so that you don't get the enum generated at the top separtaely, which I think usually looks a bit weird.
https://codereview.chromium.org/137833002/diff/270001/chrome/common/extension... File chrome/common/extensions/api/web_navigation.json (right): https://codereview.chromium.org/137833002/diff/270001/chrome/common/extension... chrome/common/extensions/api/web_navigation.json:152: "transitionType": {"type": "string", "enum": ["link", "typed", "auto_bookmark", "auto_subframe", "manual_subframe", "generated", "start_page", "form_submit", "reload", "keyword", "keyword_generated"], "description": "Cause of the navigation. The same transition types as defined in the history API are used. These are the same transition types as defined in the <a href=\"history.html#transition_types\">history API</a> except with <code>\"start_page\"</code> in place of <code>\"auto_toplevel\"</code> (for backwards compatibility)."}, On 2014/01/27 18:22:45, kalman wrote: > I thought I made this comment before actually; could you pull this transition > type data out into a type and then for each of these use $ref: TransitionType as > the types. > > You may want to annotate that TransitionType type with "inline_doc": true (I > think that's it) so that you don't get the enum generated at the top separtaely, > which I think usually looks a bit weird. I'm not sure I get your point correctly here. If not, please correct me. You means we should pull the transitionType into a new type in web_navigation.json, like: "types": [ { "id": "TransitionType", "type": "string", "enum": ["link", "typed", "auto_bookmark", "auto_subframe", "manual_subframe", "generated", "start_page", "form_submit", "reload", "keyword", "keyword_generated"], "description": "These are the same transition types as defined in the $ref:[history.VisitItem History API] except with <code>\"start_page\"</code> in place of <code>\"auto_toplevel\"</code> (for backwards compatibility)." } ], and each usage is like: "transitionType": {"$ref": "TransitionType", "description": "Cause of the navigation. The same transition types as defined in the history API are used."}, If we do it in this way, it will introduce a new type structure in webNavigation API while json schema compiler generates the bundle C++ extension API code, which will break compiling.
good point, lgtm.
(as is)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/137833002/270001
Message was sent while issue was closed.
Change committed as 247663 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
