Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23377002:
Clean up windows API implementations to use JSON compiler generated code (Closed)
|
Created:
7 years, 4 months ago by pals Modified:
7 years, 3 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionClean up windows API implementations to use JSON compiler generated code
BUG=159265
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220311
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 14
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : Rebased #Patch Set 6 : Fixing the regressions #Patch Set 7 : #
Messages
Total messages: 14 (0 generated)
Please review.
sweet cleanup https://codereview.chromium.org/23377002/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/23377002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/tabs/tabs_api.cc:120: namespace UpdateInfo = Update::Params::UpdateInfo; i really don't like all of these typedefs. Create, Get, etc are just way too generic to be pulling up into the top level namespace like this. Please just alias "namespace windows = api::windows". https://codereview.chromium.org/23377002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/tabs/tabs_api.cc:405: scoped_ptr<Create::Params> args(Create::Params::Create(*args_)); args can't be null. the schema compilation should be generating the correct code that handles whatever the schema can throw at it. the thing to null-check against is probably args->create_data not args. So: scoped_ptr<Create::Params> params(Create::Params::Create(*args_)); EXTENSION_FUNCTION_VALIDATE(params); // use this everywhere not |args| Create::Params::CreateData* create_data = params->create_data.get(); https://codereview.chromium.org/23377002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/tabs/tabs_api.cc:483: if (args->create_data->type != CreateData::TYPE_NONE) { could you restructure this to use switch()?
Fixed. PTAL https://codereview.chromium.org/23377002/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/23377002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/tabs/tabs_api.cc:120: namespace UpdateInfo = Update::Params::UpdateInfo; On 2013/08/21 15:04:36, kalman wrote: > i really don't like all of these typedefs. Create, Get, etc are just way too > generic to be pulling up into the top level namespace like this. > > Please just alias "namespace windows = api::windows". Done. https://codereview.chromium.org/23377002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/tabs/tabs_api.cc:405: scoped_ptr<Create::Params> args(Create::Params::Create(*args_)); On 2013/08/21 15:04:36, kalman wrote: > args can't be null. the schema compilation should be generating the correct code > that handles whatever the schema can throw at it. > > the thing to null-check against is probably args->create_data not args. So: > > scoped_ptr<Create::Params> params(Create::Params::Create(*args_)); > EXTENSION_FUNCTION_VALIDATE(params); > > // use this everywhere not |args| > Create::Params::CreateData* create_data = params->create_data.get(); Done. https://codereview.chromium.org/23377002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/tabs/tabs_api.cc:483: if (args->create_data->type != CreateData::TYPE_NONE) { On 2013/08/21 15:04:36, kalman wrote: > could you restructure this to use switch()? Done.
https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/tabs/tabs_api.cc (left): https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:358: if (args && args->HasKey(keys::kIncognitoKey)) { check to see how many constants like kIncognitoKey you can delete from chrome/browser/extensions/api/tabs/tabs_constants.h/cc. https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:409: if (create_data->url) { save a level of indentation here by making the condition (create_data && create_data->url) https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:415: url_strings = *create_data->url->as_strings; since as_strings isn't used again we can .swap() here rather than copying. https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:416: } no need for the {}s here, just if (...) url_strings.push_back(...); else if (...) url_strings.swap(*create_data->url->as_strings); https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:438: if (create_data) { if (create_data && create_data->tab_id) { ... } https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:493: panel_create_mode = PanelManager::CREATE_AS_DETACHED; has a 2-line condition so needs {} now. https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/tabs/tabs_api.h (right): https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.h:68: // Returns whether the window should be created in incognito mode. add comment line: |create_data| are the options passed by the extension. It may be NULL.
Please review. https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/tabs/tabs_api.cc (left): https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:358: if (args && args->HasKey(keys::kIncognitoKey)) { On 2013/08/22 15:12:11, kalman wrote: > check to see how many constants like kIncognitoKey you can delete from > chrome/browser/extensions/api/tabs/tabs_constants.h/cc. Done. https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:409: if (create_data->url) { On 2013/08/22 15:12:11, kalman wrote: > save a level of indentation here by making the condition (create_data && > create_data->url) Done. https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:415: url_strings = *create_data->url->as_strings; On 2013/08/22 15:12:11, kalman wrote: > since as_strings isn't used again we can .swap() here rather than copying. Done. https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:416: } On 2013/08/22 15:12:11, kalman wrote: > no need for the {}s here, just > > if (...) > url_strings.push_back(...); > else if (...) > url_strings.swap(*create_data->url->as_strings); Done. https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:438: if (create_data) { On 2013/08/22 15:12:11, kalman wrote: > if (create_data && create_data->tab_id) { > ... > } Done. https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:493: panel_create_mode = PanelManager::CREATE_AS_DETACHED; On 2013/08/22 15:12:11, kalman wrote: > has a 2-line condition so needs {} now. Done. https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... File chrome/browser/extensions/api/tabs/tabs_api.h (right): https://codereview.chromium.org/23377002/diff/11001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.h:68: // Returns whether the window should be created in incognito mode. On 2013/08/22 15:12:11, kalman wrote: > add comment line: > > |create_data| are the options passed by the extension. It may be NULL. Done.
lgtm https://codereview.chromium.org/23377002/diff/17001/chrome/browser/extensions... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/23377002/diff/17001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_api.cc:470: bool use_panels = false; only declare this when it's used, you can put {}s for variable declaration issues i.e. case windows::Create::Params::CreateData::TYPE_PANEL: case windows::Create::Params::CreateData::TYPE_DETACHED_PANEL: { bool use_panels = false; ... } https://codereview.chromium.org/23377002/diff/17001/chrome/browser/extensions... File chrome/browser/extensions/api/tabs/tabs_constants.h (left): https://codereview.chromium.org/23377002/diff/17001/chrome/browser/extensions... chrome/browser/extensions/api/tabs/tabs_constants.h:20: extern const char kDrawAttentionKey[]; ah well shame it isn't more.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/23377002/25001
Failed to apply patch for chrome/browser/extensions/api/tabs/tabs_api.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/extensions/api/tabs/tabs_api.cc
Hunk #1 succeeded at 110 (offset 1 line).
Hunk #2 succeeded at 224 (offset -29 lines).
Hunk #3 succeeded at 246 (offset -26 lines).
Hunk #4 succeeded at 268 (offset -26 lines).
Hunk #5 succeeded at 295 (offset -26 lines).
Hunk #6 succeeded at 321 (offset -26 lines).
Hunk #7 succeeded at 369 (offset -26 lines).
Hunk #8 succeeded at 428 (offset -26 lines).
Hunk #9 succeeded at 438 (offset -26 lines).
Hunk #10 succeeded at 457 (offset -26 lines).
Hunk #11 succeeded at 500 (offset -26 lines).
Hunk #12 FAILED at 653.
Hunk #13 succeeded at 645 (offset -26 lines).
Hunk #14 succeeded at 697 (offset -26 lines).
Hunk #15 succeeded at 729 (offset -26 lines).
Hunk #16 succeeded at 746 (offset -26 lines).
Hunk #17 FAILED at 781.
2 out of 17 hunks FAILED -- saving rejects to file
chrome/browser/extensions/api/tabs/tabs_api.cc.rej
Patch: chrome/browser/extensions/api/tabs/tabs_api.cc
Index: chrome/browser/extensions/api/tabs/tabs_api.cc
diff --git a/chrome/browser/extensions/api/tabs/tabs_api.cc
b/chrome/browser/extensions/api/tabs/tabs_api.cc
index
09f86ab18a8f11ad1dd4dda2a0c2527b6e1ecd1b..32176e7ba24784b9459edf3c0360ef5c6a9fe593
100644
--- a/chrome/browser/extensions/api/tabs/tabs_api.cc
+++ b/chrome/browser/extensions/api/tabs/tabs_api.cc
@@ -109,10 +109,7 @@ using content::WebContents;
namespace extensions {
-namespace Get = api::windows::Get;
-namespace GetAll = api::windows::GetAll;
-namespace GetCurrent = api::windows::GetCurrent;
-namespace GetLastFocused = api::windows::GetLastFocused;
+namespace windows = api::windows;
namespace errors = extension_manifest_errors;
namespace keys = tabs_constants;
namespace tabs = api::tabs;
@@ -256,7 +253,7 @@ Browser* CreateBrowserWindow(const Browser::CreateParams&
params,
// Windows
---------------------------------------------------------------------
bool WindowsGetFunction::RunImpl() {
- scoped_ptr<Get::Params> params(Get::Params::Create(*args_));
+ scoped_ptr<windows::Get::Params>
params(windows::Get::Params::Create(*args_));
EXTENSION_FUNCTION_VALIDATE(params.get());
bool populate_tabs = false;
@@ -275,7 +272,8 @@ bool WindowsGetFunction::RunImpl() {
}
bool WindowsGetCurrentFunction::RunImpl() {
- scoped_ptr<GetCurrent::Params> params(GetCurrent::Params::Create(*args_));
+ scoped_ptr<windows::GetCurrent::Params> params(
+ windows::GetCurrent::Params::Create(*args_));
EXTENSION_FUNCTION_VALIDATE(params.get());
bool populate_tabs = false;
@@ -296,8 +294,8 @@ bool WindowsGetCurrentFunction::RunImpl() {
}
bool WindowsGetLastFocusedFunction::RunImpl() {
- scoped_ptr<GetLastFocused::Params> params(
- GetLastFocused::Params::Create(*args_));
+ scoped_ptr<windows::GetLastFocused::Params> params(
+ windows::GetLastFocused::Params::Create(*args_));
EXTENSION_FUNCTION_VALIDATE(params.get());
bool populate_tabs = false;
@@ -323,7 +321,8 @@ bool WindowsGetLastFocusedFunction::RunImpl() {
}
bool WindowsGetAllFunction::RunImpl() {
- scoped_ptr<GetAll::Params> params(GetAll::Params::Create(*args_));
+ scoped_ptr<windows::GetAll::Params> params(
+ windows::GetAll::Params::Create(*args_));
EXTENSION_FUNCTION_VALIDATE(params.get());
bool populate_tabs = false;
@@ -348,16 +347,14 @@ bool WindowsGetAllFunction::RunImpl() {
}
bool WindowsCreateFunction::ShouldOpenIncognitoWindow(
- const base::DictionaryValue* args,
- std::vector<GURL>* urls,
- bool* is_error) {
+ const windows::Create::Params::CreateData* create_data,
+ std::vector<GURL>* urls, bool* is_error) {
*is_error = false;
const IncognitoModePrefs::Availability incognito_availability =
IncognitoModePrefs::GetAvailability(profile_->GetPrefs());
bool incognito = false;
- if (args && args->HasKey(keys::kIncognitoKey)) {
- EXTENSION_FUNCTION_VALIDATE(args->GetBoolean(keys::kIncognitoKey,
- &incognito));
+ if (create_data && create_data->incognito) {
+ incognito = *create_data->incognito;
if (incognito && incognito_availability == IncognitoModePrefs::DISABLED) {
error_ = keys::kIncognitoModeIsDisabled;
*is_error = true;
@@ -398,69 +395,49 @@ bool WindowsCreateFunction::ShouldOpenIncognitoWindow(
}
bool WindowsCreateFunction::RunImpl() {
- base::DictionaryValue* args = NULL;
+ scoped_ptr<windows::Create::Params> params(
+ windows::Create::Params::Create(*args_));
+ EXTENSION_FUNCTION_VALIDATE(params);
std::vector<GURL> urls;
TabStripModel* source_tab_strip = NULL;
int tab_index = -1;
- if (HasOptionalArgument(0))
- EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &args));
+ windows::Create::Params::CreateData* create_data = params->create_data.get();
// Look for optional url.
- if (args) {
- if (args->HasKey(keys::kUrlKey)) {
- Value* url_value;
- std::vector<std::string> url_strings;
- args->Get(keys::kUrlKey, &url_value);
-
- // First, get all the URLs the client wants to open.
- if (url_value->IsType(Value::TYPE_STRING)) {
- std::string url_string;
- url_value->GetAsString(&url_string);
- url_strings.push_back(url_string);
- } else if (url_value->IsType(Value::TYPE_LIST)) {
- const base::ListValue* url_list =
- static_cast<const base::ListValue*>(url_value);
- for (size_t i = 0; i < url_list->GetSize(); ++i) {
- std::string url_string;
- EXTENSION_FUNCTION_VALIDATE(url_list->GetString(i, &url_string));
- url_strings.push_back(url_string);
- }
+ if (create_data && create_data->url) {
+ std::vector<std::string> url_strings;
+ // First, get all the URLs the client wants to open.
+ if (create_data->url->as_string)
+ url_strings.push_back(*create_data->url->as_string);
+ else if (create_data->url->as_strings)
+ url_strings.swap(*create_data->url->as_strings);
+
+ // Second, resolve, validate and convert them to GURLs.
+ for (std::vector<std::string>::iterator i = url_strings.begin();
+ i != url_strings.end(); ++i) {
+ GURL url = ExtensionTabUtil::ResolvePossiblyRelativeURL(
+ *i, GetExtension());
+ if (!url.is_valid()) {
+ error_ = ErrorUtils::FormatErrorMessage(keys::kInvalidUrlError, *i);
+ return false;
}
-
- // Second, resolve, validate and convert them to GURLs.
- for (std::vector<std::string>::iterator i = url_strings.begin();
- i != url_strings.end(); ++i) {
- GURL url = ExtensionTabUtil::ResolvePossiblyRelativeURL(
- *i, GetExtension());
- if (!url.is_valid()) {
- error_ = ErrorUtils::FormatErrorMessage(
- keys::kInvalidUrlError, *i);
- return false;
- }
- // Don't let the extension crash the browser or renderers.
- if (ExtensionTabUtil::IsCrashURL(url)) {
- error_ = keys::kNoCrashBrowserError;
- return false;
- }
- urls.push_back(url);
+ // Don't let the extension crash the browser or renderers.
+ if (ExtensionTabUtil::IsCrashURL(url)) {
+ error_ = keys::kNoCrashBrowserError;
+ return false;
}
+ urls.push_back(url);
}
}
// Look for optional tab id.
- if (args) {
- int tab_id = -1;
- if (args->HasKey(keys::kTabIdKey)) {
- EXTENSION_FUNCTION_VALIDATE(args->GetInteger(keys::kTabIdKey, &tab_id));
-
- // Find the tab. |source_tab_strip| and |tab_index| will later be used to
- // move the tab into the created window.
- if (!GetTabById(tab_id, profile(), include_incognito(),
- NULL, &source_tab_strip,
- NULL, &tab_index, &error_))
- return false;
- }
+ if (create_data && create_data->tab_id) {
+ // Find the tab. |source_tab_strip| and |tab_index| will later be used to
+ // move the tab into the created window.
+ if (!GetTabById(*create_data->tab_id, profile(), include_incognito(), NULL,
+ &source_tab_strip, NULL, &tab_index, &error_))
+ return false;
}
Profile* window_profile = profile();
@@ -477,7 +454,7 @@ bool WindowsCreateFunction::RunImpl() {
// Decide whether we are opening a normal window or an incognito window.
bool is_error = true;
- bool open_incognito_window = ShouldOpenIncognitoWindow(args, &urls,
+ bool open_incognito_window = ShouldOpenIncognitoWindow(create_data, &urls,
&is_error);
if (is_error) {
// error_ member variable is set inside of ShouldOpenIncognitoWindow.
@@ -487,18 +464,16 @@ bool WindowsCreateFunction::RunImpl() {
window_profile = window_profile->GetOffTheRecordProfile();
}
- if (args) {
+ if (create_data) {
// Figure out window type before figuring out bounds so that default
// bounds can be set according to the window type.
- std::string type_str;
- if (args->HasKey(keys::kWindowTypeKey)) {
- EXTENSION_FUNCTION_VALIDATE(args->GetString(keys::kWindowTypeKey,
- &type_str));
- if (type_str == keys::kWindowTypeValuePopup) {
+ switch (create_data->type) {
+ case windows::Create::Params::CreateData::TYPE_POPUP:
window_type = Browser::TYPE_POPUP;
extension_id = GetExtension()->id();
- } else if (type_str == keys::kWindowTypeValuePanel ||
- type_str == keys::kWindowTypeValueDetachedPanel) {
+ break;
+ case windows::Create::Params::CreateData::TYPE_PANEL:
+ case windows::Create::Params::CreateData::TYPE_DETACHED_PANEL: {
extension_id = GetExtension()->id();
bool use_panels = false;
#if !defined(OS_ANDROID)
@@ …
(message too large)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/23377002/31001
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/23377002/53001
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/23377002/74001
Message was sent while issue was closed.
Change committed as 220311 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
