Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(310)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 10878040: Move context-sensitive app.window.* functions to app.window.current().* (Closed)

Created:
8 years, 4 months ago by jeremya
Modified:
8 years, 3 months ago
CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, benwells
Visibility:
Public.

Description

Move context-sensitive app.window.* functions to app.window.current().* This changes app.window.create() to call back with a wrapper AppWindow object instead of a direct reference to the DOM of the child window. The DOM is still accessible via child.dom. chrome.app.window.create('child.html', function (child) { child.dom.onload = function () { ... } child.maximize(); }) R=mihaip@chromium.org,kalman@chromium.org Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154631

Patch Set 1 #

Total comments: 10

Patch Set 2 : comments #

Total comments: 2

Patch Set 3 : add dom property #

Total comments: 16

Patch Set 4 : comments #

Patch Set 5 : fix tests #

Total comments: 5

Patch Set 6 : nits #

Patch Set 7 : fix test #

Patch Set 8 : rebase #

Patch Set 9 : fix #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+199 lines, -112 lines) Patch
A chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.h View 1 1 chunk +67 lines, -0 lines 0 comments Download
A chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc View 1 2 3 4 5 1 chunk +59 lines, -0 lines 0 comments Download
M chrome/browser/extensions/api/app_window/app_window_api.h View 2 chunks +0 lines, -49 lines 0 comments Download
M chrome/browser/extensions/api/app_window/app_window_api.cc View 1 2 3 4 5 6 7 3 chunks +0 lines, -41 lines 0 comments Download
M chrome/chrome_browser_extensions.gypi View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/common/extensions/api/api.gyp View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
A chrome/common/extensions/api/app_current_window_internal.idl View 1 2 3 1 chunk +21 lines, -0 lines 0 comments Download
M chrome/common/extensions/api/app_window.idl View 1 2 3 4 5 6 7 2 chunks +24 lines, -7 lines 0 comments Download
M chrome/common/extensions/extension.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/extensions/permissions/api_permission.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/extensions/permissions/api_permission.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/extensions/permissions/permission_set_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M chrome/renderer/resources/extensions/app_window_custom_bindings.js View 1 2 3 2 chunks +12 lines, -6 lines 2 comments Download
M chrome/test/data/extensions/platform_apps/windows_api/test.js View 1 2 3 4 2 chunks +9 lines, -9 lines 0 comments Download

Messages

Total messages: 28 (0 generated)
jeremya
8 years, 4 months ago (2012-08-24 00:48:30 UTC) #1
Mihai Parparita -not on Chrome
Seems like you'd want to update chrome/test/data/extensions/platform_apps/windows_api/test.js, and possibly other test code. Relatedly, it would ...
8 years, 4 months ago (2012-08-24 01:05:06 UTC) #2
benwells
https://chromiumcodereview.appspot.com/10878040/diff/1/chrome/common/extensions/api/app_current_window.idl File chrome/common/extensions/api/app_current_window.idl (right): https://chromiumcodereview.appspot.com/10878040/diff/1/chrome/common/extensions/api/app_current_window.idl#newcode19 chrome/common/extensions/api/app_current_window.idl:19: static void restore(); Maybe I am not understanding the ...
8 years, 4 months ago (2012-08-24 01:11:55 UTC) #3
jeremya
https://chromiumcodereview.appspot.com/10878040/diff/1/chrome/common/extensions/api/app_current_window.idl File chrome/common/extensions/api/app_current_window.idl (right): https://chromiumcodereview.appspot.com/10878040/diff/1/chrome/common/extensions/api/app_current_window.idl#newcode7 chrome/common/extensions/api/app_current_window.idl:7: [internal] namespace app.currentWindow { On 2012/08/24 01:05:07, Mihai Parparita ...
8 years, 4 months ago (2012-08-24 02:10:08 UTC) #4
Mihai Parparita -not on Chrome
https://chromiumcodereview.appspot.com/10878040/diff/7001/chrome/renderer/resources/extensions/app_window_custom_bindings.js File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://chromiumcodereview.appspot.com/10878040/diff/7001/chrome/renderer/resources/extensions/app_window_custom_bindings.js#newcode28 chrome/renderer/resources/extensions/app_window_custom_bindings.js:28: AppWindow.prototype.moveTo = window.moveTo.bind(window); Will this do the right thing ...
8 years, 4 months ago (2012-08-24 17:58:09 UTC) #5
jeremya
https://chromiumcodereview.appspot.com/10878040/diff/7001/chrome/renderer/resources/extensions/app_window_custom_bindings.js File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://chromiumcodereview.appspot.com/10878040/diff/7001/chrome/renderer/resources/extensions/app_window_custom_bindings.js#newcode28 chrome/renderer/resources/extensions/app_window_custom_bindings.js:28: AppWindow.prototype.moveTo = window.moveTo.bind(window); On 2012/08/24 17:58:09, Mihai Parparita wrote: ...
8 years, 3 months ago (2012-08-26 23:49:37 UTC) #6
not at google - send to devlin
drive-by and removing myself as reviewer since mihai has it covered. https://chromiumcodereview.appspot.com/10878040/diff/9001/chrome/common/extensions/extension.cc File chrome/common/extensions/extension.cc (right): ...
8 years, 3 months ago (2012-08-27 01:51:59 UTC) #7
Mihai Parparita -not on Chrome
LGTM, but tests still need to be updated. https://chromiumcodereview.appspot.com/10878040/diff/9001/chrome/common/extensions/api/app_current_window_internal.idl File chrome/common/extensions/api/app_current_window_internal.idl (right): https://chromiumcodereview.appspot.com/10878040/diff/9001/chrome/common/extensions/api/app_current_window_internal.idl#newcode7 chrome/common/extensions/api/app_current_window_internal.idl:7: [internal,nodoc] ...
8 years, 3 months ago (2012-08-27 23:21:06 UTC) #8
jeremya
http://codereview.chromium.org/10878040/diff/9001/chrome/common/extensions/api/app_current_window_internal.idl File chrome/common/extensions/api/app_current_window_internal.idl (right): http://codereview.chromium.org/10878040/diff/9001/chrome/common/extensions/api/app_current_window_internal.idl#newcode7 chrome/common/extensions/api/app_current_window_internal.idl:7: [internal,nodoc] namespace app.currentWindowInternal { On 2012/08/27 23:21:06, Mihai Parparita ...
8 years, 3 months ago (2012-08-30 02:38:53 UTC) #9
jeremya
+jhawkins for chrome/ OWNERS
8 years, 3 months ago (2012-08-30 05:22:44 UTC) #10
James Hawkins
LGTM with nits. https://chromiumcodereview.appspot.com/10878040/diff/20001/chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc File chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc (right): https://chromiumcodereview.appspot.com/10878040/diff/20001/chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc#newcode12 chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:12: namespace { nit: Blank line after ...
8 years, 3 months ago (2012-08-30 13:45:03 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/10878040/24001
8 years, 3 months ago (2012-08-31 07:51:25 UTC) #12
commit-bot: I haz the power
Try job failure for 10878040-24001 (retry) on mac_rel for step "unit_tests". It's a second try, ...
8 years, 3 months ago (2012-08-31 09:14:49 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/10878040/28002
8 years, 3 months ago (2012-09-01 00:35:15 UTC) #14
commit-bot: I haz the power
Failed to apply patch for chrome/browser/extensions/api/app_window/app_window_api.cc: While running patch -p1 --forward --force; patching file chrome/browser/extensions/api/app_window/app_window_api.cc ...
8 years, 3 months ago (2012-09-01 00:35:18 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/10878040/26003
8 years, 3 months ago (2012-09-01 02:37:49 UTC) #16
commit-bot: I haz the power
Try job failure for 10878040-26003 (retry) on linux_clang for step "compile" (clobber build). It's a ...
8 years, 3 months ago (2012-09-01 03:02:59 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/10878040/26005
8 years, 3 months ago (2012-09-01 04:17:01 UTC) #18
commit-bot: I haz the power
Try job failure for 10878040-26005 (retry) on linux_rel for step "interactive_ui_tests". It's a second try, ...
8 years, 3 months ago (2012-09-01 06:20:57 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/10878040/26005
8 years, 3 months ago (2012-09-01 06:39:36 UTC) #20
commit-bot: I haz the power
Try job failure for 10878040-26005 (retry) on linux_chromeos for step "browser_tests". It's a second try, ...
8 years, 3 months ago (2012-09-01 10:56:30 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/10878040/26005
8 years, 3 months ago (2012-09-02 00:56:08 UTC) #22
commit-bot: I haz the power
Change committed as 154631
8 years, 3 months ago (2012-09-02 03:06:44 UTC) #23
Aaron Boodman
https://chromiumcodereview.appspot.com/10878040/diff/26005/chrome/renderer/resources/extensions/app_window_custom_bindings.js File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://chromiumcodereview.appspot.com/10878040/diff/26005/chrome/renderer/resources/extensions/app_window_custom_bindings.js#newcode31 chrome/renderer/resources/extensions/app_window_custom_bindings.js:31: AppWindow.prototype.dom = window; Nit: Not actually the 'dom', but ...
8 years, 3 months ago (2012-09-06 04:53:29 UTC) #24
jeremya
https://chromiumcodereview.appspot.com/10878040/diff/26005/chrome/renderer/resources/extensions/app_window_custom_bindings.js File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://chromiumcodereview.appspot.com/10878040/diff/26005/chrome/renderer/resources/extensions/app_window_custom_bindings.js#newcode31 chrome/renderer/resources/extensions/app_window_custom_bindings.js:31: AppWindow.prototype.dom = window; On 2012/09/06 04:53:29, Aaron Boodman wrote: ...
8 years, 3 months ago (2012-09-06 16:31:20 UTC) #25
Mihai Parparita -not on Chrome
On Thu, Sep 6, 2012 at 9:31 AM, <jeremya@chromium.org> wrote: > Perhaps I should rename ...
8 years, 3 months ago (2012-09-06 17:40:20 UTC) #26
jeremya
We could call it contentWindow? On Thu, Sep 6, 2012 at 10:40 AM, Mihai Parparita ...
8 years, 3 months ago (2012-09-06 17:44:07 UTC) #27
Mihai Parparita -not on Chrome
8 years, 3 months ago (2012-09-06 18:07:22 UTC) #28 
Might as well.

On Thu, Sep 6, 2012 at 10:43 AM, Jeremy Apthorp <jeremya@chromium.org>wrote:

> We could call it contentWindow?
>
>
> On Thu, Sep 6, 2012 at 10:40 AM, Mihai Parparita <mihaip@chromium.org>wrote:
>
>> On Thu, Sep 6, 2012 at 9:31 AM, <jeremya@chromium.org> wrote:
>>
>>> Perhaps I should rename it to 'global'?
>>>
>>
>> Or domWindow (global is more correct, but this is closer to the
>> contentWindow name used for iframes)?
>>
>> Mihai
>>
>
>

Powered by Google App Engine
This is Rietveld 408576698