Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16975007:
Run shim's watchForTag on document.DOMContentLoaded (Closed)
|
Created:
7 years, 6 months ago by lazyboy Modified:
7 years, 6 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRun shim's watchForTag on document.DOMContentLoaded
In shim (web_view.js) we used to register watchForTag (L1) like this:
window.addEventListener('DOMContentLoaded', L1);
Let's assume in chrome app window we have
document.addEventListener('DOMContentLoaded', L2);
Problem is L2 fires before L1 and we want to run shim code before L2. Otherwise L2 can redefine property on WebView instance. This change makes sure watchForTag would be called before L2 fires.
BUG=251965
Test=WebViewTest.SetPropertyOnDocumentReady updated to cover the case.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207423
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address comments from kalman. #
Total comments: 7
Patch Set 3 : Not using current. #
Total comments: 10
Patch Set 4 : Address comments from kalman@ #
Total comments: 2
Patch Set 5 : rebase + add test code from @kalman's comment #
Total comments: 2
Patch Set 6 : Add more debug code. #Patch Set 7 : use window.readystatechange as adamk@ suggested, remove debug code #Patch Set 8 : Use correct comment instead of copy/pasting. #
Total comments: 2
Patch Set 9 : Remove unrelated changes + add tests. #
Total comments: 4
Patch Set 10 : Address comments from kalman@ #
Total comments: 2
Patch Set 11 : Address comments from kalman@ #
Messages
Total messages: 40 (0 generated)
approach seems fine though I only have a very vague understanding of what the purpose here is. https://codereview.chromium.org/16975007/diff/1/chrome/renderer/extensions/ap... File chrome/renderer/extensions/app_window_custom_bindings.cc (right): https://codereview.chromium.org/16975007/diff/1/chrome/renderer/extensions/ap... chrome/renderer/extensions/app_window_custom_bindings.cc:70: LoadWatcher(v8::Isolate* isolate, looks like isolate isn't used https://codereview.chromium.org/16975007/diff/1/chrome/renderer/extensions/ap... chrome/renderer/extensions/app_window_custom_bindings.cc:107: v8::Handle<v8::Value> AppWindowCustomBindings::OnContextReady( this doesn't actually have anything to do with app windows; nor is it about a context being ready, it's about a document being ready. also the code is a little bit wrong. it should probably be moved elsewhere particularly if you're going to use it in web view. maybe a RenderViewObserverNatives class or something. anyhow, see how it turns out. https://codereview.chromium.org/16975007/diff/1/chrome/renderer/extensions/ap... chrome/renderer/extensions/app_window_custom_bindings.cc:136: return v8::Undefined(); just CHECK these, we call the method and it'd be a bug to call it wrongly. I see it's being done above; may as well change that too https://codereview.chromium.org/16975007/diff/1/chrome/renderer/extensions/ap... chrome/renderer/extensions/app_window_custom_bindings.cc:144: content::RenderView* view = content::RenderView::FromRoutingID(view_id); why go render view -> routing id -> render view here? all you need is context()->RenderView() right? https://codereview.chromium.org/16975007/diff/1/chrome/renderer/extensions/ap... chrome/renderer/extensions/app_window_custom_bindings.cc:151: return v8::True(); might as well return undefined again I see that code above it does this too, but that is unnecessarily verbose, so yeah, feel free to fix it https://codereview.chromium.org/16975007/diff/1/chrome/renderer/resources/ext... File chrome/renderer/resources/extensions/web_view.js (right): https://codereview.chromium.org/16975007/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/web_view.js:388: window.console.log('***** current: ' + w); i presume this doesn't really do anything? https://codereview.chromium.org/16975007/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/web_view.js:392: document.addEventListener('DOMContentLoaded', function() { from reading the code it looks like this extra listener isn't necessary? I guess I don't know the context of this change, but this callback is run when the document is ready; does that not imply DOMContentLoaded? I.e. can you just add watchForTag immediately.
My basic question about the fact still remains: chrome.app.window.create is called. web_view.js scripts loads, in document A then app loads document B and all dom elements in the app goes to B. Why is A != B, or rather, shall B always be a new document than A? https://codereview.chromium.org/16975007/diff/1/chrome/renderer/extensions/ap... File chrome/renderer/extensions/app_window_custom_bindings.cc (right): https://codereview.chromium.org/16975007/diff/1/chrome/renderer/extensions/ap... chrome/renderer/extensions/app_window_custom_bindings.cc:70: LoadWatcher(v8::Isolate* isolate, On 2013/06/13 22:08:32, kalman wrote: > looks like isolate isn't used Removed. https://codereview.chromium.org/16975007/diff/1/chrome/renderer/extensions/ap... chrome/renderer/extensions/app_window_custom_bindings.cc:107: v8::Handle<v8::Value> AppWindowCustomBindings::OnContextReady( On 2013/06/13 22:08:32, kalman wrote: > this doesn't actually have anything to do with app windows; nor is it about a > context being ready, it's about a document being ready. also the code is a > little bit wrong. it should probably be moved elsewhere particularly if you're > going to use it in web view. > > maybe a RenderViewObserverNatives class or something. anyhow, see how it turns > out. I've moved to a separate class RenderViewObserverNatives, exported it as renderViewObserverNatives. Better naming suggestions welcome (e.g. OnDocumentCreatedForCurrentContext... long) https://codereview.chromium.org/16975007/diff/1/chrome/renderer/extensions/ap... chrome/renderer/extensions/app_window_custom_bindings.cc:136: return v8::Undefined(); On 2013/06/13 22:08:32, kalman wrote: > just CHECK these, we call the method and it'd be a bug to call it wrongly. > > I see it's being done above; may as well change that too Done. https://codereview.chromium.org/16975007/diff/1/chrome/renderer/extensions/ap... chrome/renderer/extensions/app_window_custom_bindings.cc:144: content::RenderView* view = content::RenderView::FromRoutingID(view_id); On 2013/06/13 22:08:32, kalman wrote: > why go render view -> routing id -> render view here? all you need is > context()->RenderView() right? Ah I was quick hacking, fixed. https://codereview.chromium.org/16975007/diff/1/chrome/renderer/extensions/ap... chrome/renderer/extensions/app_window_custom_bindings.cc:151: return v8::True(); On 2013/06/13 22:08:32, kalman wrote: > might as well return undefined > > again I see that code above it does this too, but that is unnecessarily verbose, > so yeah, feel free to fix it Looks like the return value is used in https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... I can remove this one though, not sure if being inconsistent with the code above is OK. https://codereview.chromium.org/16975007/diff/1/chrome/renderer/resources/ext... File chrome/renderer/resources/extensions/web_view.js (right): https://codereview.chromium.org/16975007/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/web_view.js:388: window.console.log('***** current: ' + w); On 2013/06/13 22:08:32, kalman wrote: > i presume this doesn't really do anything? Right, debug code, removed. https://codereview.chromium.org/16975007/diff/1/chrome/renderer/resources/ext... chrome/renderer/resources/extensions/web_view.js:392: document.addEventListener('DOMContentLoaded', function() { On 2013/06/13 22:08:32, kalman wrote: > from reading the code it looks like this extra listener isn't necessary? I guess > I don't know the context of this change, but this callback is run when the > document is ready; does that not imply DOMContentLoaded? I.e. can you just add > watchForTag immediately. This is the point where (new) document is created. document.readyState == 'loading' at this point we want document.readyState = 'interactive' or 'complete', so we need this listener.
On 2013/06/14 00:09:50, lazyboy wrote: > My basic question about the fact still remains: > > chrome.app.window.create is called. > web_view.js scripts loads, in document A > then app loads document B and all dom elements in the app goes to B. > > Why is A != B, or rather, shall B always be a new document than A? > I don't quite follow the steps here. What is A and what is B?
On 2013/06/14 00:19:39, kalman wrote: > On 2013/06/14 00:09:50, lazyboy wrote: > > My basic question about the fact still remains: > > > > chrome.app.window.create is called. > > web_view.js scripts loads, in document A > > then app loads document B and all dom elements in the app goes to B. > > > > Why is A != B, or rather, shall B always be a new document than A? > > > > I don't quite follow the steps here. What is A and what is B? I used A and B to name the "document" object and to emphasize they are different "document"s.
could you add comments which documents you're referring to?
Added comments for A and B. https://codereview.chromium.org/16975007/diff/5001/chrome/renderer/resources/... File chrome/renderer/resources/extensions/web_view.js (right): https://codereview.chromium.org/16975007/diff/5001/chrome/renderer/resources/... chrome/renderer/resources/extensions/web_view.js:49: If i refer to "document" here (in file's global scope), this would be document A. i.e. var documentA = document; https://codereview.chromium.org/16975007/diff/5001/chrome/renderer/resources/... chrome/renderer/resources/extensions/web_view.js:54: document.addEventListener('DOMContentLoaded', function(e) { (See this after next comment) this "document" is documentB. https://codereview.chromium.org/16975007/diff/5001/chrome/renderer/resources/... chrome/renderer/resources/extensions/web_view.js:59: /** (This is not about here) Let's say I have an app background.js: ..onLaunched.addListener(function() { chrome.app.window.create('main.html'); }); main.html: <script src="main.js"></script> main.js: var documentB = document; // <-- this
https://codereview.chromium.org/16975007/diff/5001/chrome/renderer/extensions... File chrome/renderer/extensions/render_view_observer_natives.cc (right): https://codereview.chromium.org/16975007/diff/5001/chrome/renderer/extensions... chrome/renderer/extensions/render_view_observer_natives.cc:40: v8::Local<v8::Context> context = frame->mainWorldScriptContext(); I suspect it has something to do with this context switching here. Try the following: 1. Pass the ChromeV8Context instance into this LoadWatcher class. 2. Delete basically all of this and just call chrome_v8_context_->CallFunction(...) which does all the scoping for you. no handle scope, context scope, etc.
On 2013/06/14 00:58:31, kalman wrote: > https://codereview.chromium.org/16975007/diff/5001/chrome/renderer/extensions... > File chrome/renderer/extensions/render_view_observer_natives.cc (right): > > https://codereview.chromium.org/16975007/diff/5001/chrome/renderer/extensions... > chrome/renderer/extensions/render_view_observer_natives.cc:40: > v8::Local<v8::Context> context = frame->mainWorldScriptContext(); > I suspect it has something to do with this context switching here. Try the > following: > > 1. Pass the ChromeV8Context instance into this LoadWatcher class. > 2. Delete basically all of this and just call > chrome_v8_context_->CallFunction(...) which does all the scoping for you. > > no handle scope, context scope, etc. Seems I cannot create the args without a handle scope (and hence I need context scope): "V8 error: Cannot create a handle without a HandleScope" Also, the two "document" issue I was talking about happens regardless of the callback here. It simply happens as is, without this CL.
https://chromiumcodereview.appspot.com/16975007/diff/5001/chrome/renderer/ext... File chrome/renderer/extensions/render_view_observer_natives.cc (right): https://chromiumcodereview.appspot.com/16975007/diff/5001/chrome/renderer/ext... chrome/renderer/extensions/render_view_observer_natives.cc:48: callback_->Call(global, 1, args); To answer your comment: The problem I think is here. frame->mainWorldScriptContext is the context of the frame which just loaded, not the frame that the callback was used in. It's conceivable that this results in window.document being different, unexpectedly (likely the "window" comes from the global of whatever context has been entered i.e. is in scope). If you were to save the calling context i.e. pass ChromeV8Context in here as I suggested, then use v8::Context::Scope scope(context->v8_context()) then I think it would work. Hopefully. That's something to try. However, this can all be v8::HandleScope handle_scope; context_->CallFunction(callback_, arraysize(args), args); I think the reason it didn't work before is also because of this?
https://chromiumcodereview.appspot.com/16975007/diff/5001/chrome/renderer/ext... File chrome/renderer/extensions/render_view_observer_natives.cc (right): https://chromiumcodereview.appspot.com/16975007/diff/5001/chrome/renderer/ext... chrome/renderer/extensions/render_view_observer_natives.cc:48: callback_->Call(global, 1, args); On 2013/06/14 05:26:27, kalman wrote: > To answer your comment: > > The problem I think is here. frame->mainWorldScriptContext is the context of the > frame which just loaded, not the frame that the callback was used in. Why are there two frames? My assumption was the chrome/renderer/resources/... JavaScript resources run on the same document (or should I say frame?) as the one in created via chrome.app.window.create(), which I see is probably not true, but wanted to know why. It's > conceivable that this results in window.document being different, unexpectedly > (likely the "window" comes from the global of whatever context has been entered > i.e. is in scope). > > If you were to save the calling context i.e. pass ChromeV8Context in here as I > suggested, then use v8::Context::Scope scope(context->v8_context()) then I think > it would work. Hopefully. That's something to try. However, this can all be > > v8::HandleScope handle_scope; > context_->CallFunction(callback_, arraysize(args), args); > > I think the reason it didn't work before is also because of this? I'm using RenderViewObserverNative's context instead of mainWorldScriptContext, now it doesn't crash as I'm creating the handle scope with the context as you suggested, but I keep seeing the same behavior. See latest logging changes in web_view.js, the app always sees documentB, the renderer script sees documentA, I want to know when documentB's DOMContentLoaded fires (that part is working with the change).
This looks right to me, I don't understand why documentA != documentB there. Adam is there any chance you could look at it quickly? https://chromiumcodereview.appspot.com/16975007/diff/5001/chrome/renderer/ext... File chrome/renderer/extensions/render_view_observer_natives.cc (right): https://chromiumcodereview.appspot.com/16975007/diff/5001/chrome/renderer/ext... chrome/renderer/extensions/render_view_observer_natives.cc:48: callback_->Call(global, 1, args); > Why are there two frames? My assumption was the chrome/renderer/resources/... > JavaScript resources run on the same document (or should I say frame?) as the > one in created via chrome.app.window.create(), which I see is probably not true, > but wanted to know why. The first frame is the one that created the app window, the second is the one inside the app window. https://chromiumcodereview.appspot.com/16975007/diff/17001/chrome/renderer/ex... File chrome/renderer/extensions/render_view_observer_natives.cc (right): https://chromiumcodereview.appspot.com/16975007/diff/17001/chrome/renderer/ex... chrome/renderer/extensions/render_view_observer_natives.cc:41: void CallbackAndDie(WebKit::WebFrame* frame, bool succeeded) { frame not used https://chromiumcodereview.appspot.com/16975007/diff/17001/chrome/renderer/ex... chrome/renderer/extensions/render_view_observer_natives.cc:43: v8::Context::Scope scope(context_->v8_context()); you don't need the scope, CallFunction does it for you https://chromiumcodereview.appspot.com/16975007/diff/17001/chrome/renderer/ex... chrome/renderer/extensions/render_view_observer_natives.cc:45: succeeded ? v8::True() : v8::False() v8::Boolean::New(succeeded) https://chromiumcodereview.appspot.com/16975007/diff/17001/chrome/renderer/ex... chrome/renderer/extensions/render_view_observer_natives.cc:91: if (!view) I think you can assume there's a render view here. https://chromiumcodereview.appspot.com/16975007/diff/17001/chrome/renderer/re... File chrome/renderer/resources/extensions/web_view.js (right): https://chromiumcodereview.appspot.com/16975007/diff/17001/chrome/renderer/re... chrome/renderer/resources/extensions/web_view.js:10: var appWindowNatives = requireNative('app_window_natives'); don't need this anymore
https://chromiumcodereview.appspot.com/16975007/diff/17001/chrome/renderer/ex... File chrome/renderer/extensions/render_view_observer_natives.cc (right): https://chromiumcodereview.appspot.com/16975007/diff/17001/chrome/renderer/ex... chrome/renderer/extensions/render_view_observer_natives.cc:41: void CallbackAndDie(WebKit::WebFrame* frame, bool succeeded) { On 2013/06/14 16:45:27, kalman wrote: > frame not used Done. https://chromiumcodereview.appspot.com/16975007/diff/17001/chrome/renderer/ex... chrome/renderer/extensions/render_view_observer_natives.cc:43: v8::Context::Scope scope(context_->v8_context()); On 2013/06/14 16:45:27, kalman wrote: > you don't need the scope, CallFunction does it for you Done. https://chromiumcodereview.appspot.com/16975007/diff/17001/chrome/renderer/ex... chrome/renderer/extensions/render_view_observer_natives.cc:45: succeeded ? v8::True() : v8::False() On 2013/06/14 16:45:27, kalman wrote: > v8::Boolean::New(succeeded) Done. https://chromiumcodereview.appspot.com/16975007/diff/17001/chrome/renderer/ex... chrome/renderer/extensions/render_view_observer_natives.cc:91: if (!view) On 2013/06/14 16:45:27, kalman wrote: > I think you can assume there's a render view here. Done. https://chromiumcodereview.appspot.com/16975007/diff/17001/chrome/renderer/re... File chrome/renderer/resources/extensions/web_view.js (right): https://chromiumcodereview.appspot.com/16975007/diff/17001/chrome/renderer/re... chrome/renderer/resources/extensions/web_view.js:10: var appWindowNatives = requireNative('app_window_natives'); On 2013/06/14 16:45:27, kalman wrote: > don't need this anymore Done.
Happy to take a look, but I need some context here. A more detailed change description would help a lot.
On 2013/06/18 19:01:21, adamk wrote: > Happy to take a look, but I need some context here. A more detailed change > description would help a lot. I've updated the description.
https://chromiumcodereview.appspot.com/16975007/diff/20001/chrome/renderer/re... File chrome/renderer/resources/extensions/web_view.js (right): https://chromiumcodereview.appspot.com/16975007/diff/20001/chrome/renderer/re... chrome/renderer/resources/extensions/web_view.js:52: if (documentA == documentB) { This only makes sense to me if documentA is undefined here. Is it?
Also, it's not clear to me that (i) is necessary at all, since MutationObservers run during page load as well. So adding a MutationObserver to the document in a didCreateDocumentElement() callback should be sufficient to catch all <webview> tags, either ones existing in the markup during load or added later.
On 2013/06/18 22:38:06, adamk wrote: > Also, it's not clear to me that (i) is necessary at all, since MutationObservers > run during page load as well. So adding a MutationObserver to the document in a > didCreateDocumentElement() callback should be sufficient to catch all <webview> > tags, either ones existing in the markup during load or added later. OK, I can fix (i) in a separate go once we have this issue resolved.
And missed draft publishing... https://chromiumcodereview.appspot.com/16975007/diff/20001/chrome/renderer/re... File chrome/renderer/resources/extensions/web_view.js (right): https://chromiumcodereview.appspot.com/16975007/diff/20001/chrome/renderer/re... chrome/renderer/resources/extensions/web_view.js:52: if (documentA == documentB) { On 2013/06/18 22:26:49, adamk wrote: > This only makes sense to me if documentA is undefined here. Is it? Surprisingly it is not, I tried to addEventListener to it and that didn't throw any exception. I just double checked now trying to window.console.log(documentA) and it prints [object HTMLDocumentElement].
As a possibly-aside, I messed up ChromeV8Context::CallFunction and added a HandleScope there instead of a Context::Scope. Does fixing that make any difference?
FYI, here's a test app that i'm using: https://github.com/lazyboy/chromium/tree/master/tests/chrome-apps/webview_doc... note that for this app, web_view.js loads twice in total: 1) for the background script: test.js The background script creates an app window (main.html) 2) the chrome app window (main.html and main.js). This app window is created from the background script. The document difference is in #2. https://chromiumcodereview.appspot.com/16975007/diff/25002/chrome/renderer/ex... File chrome/renderer/extensions/chrome_v8_context.cc (right): https://chromiumcodereview.appspot.com/16975007/diff/25002/chrome/renderer/ex... chrome/renderer/extensions/chrome_v8_context.cc:80: v8::Context::Scope scope(v8_context()); @kalman, did you mean here? How can I (or do I need to?) get rid of handle_scope here? I'm still seeing the same behavior for document-s.
Ok. Adam is afk for the rest of the day, I'll hopefully have another look at it with him tomorrow. https://chromiumcodereview.appspot.com/16975007/diff/25002/chrome/renderer/ex... File chrome/renderer/extensions/chrome_v8_context.cc (right): https://chromiumcodereview.appspot.com/16975007/diff/25002/chrome/renderer/ex... chrome/renderer/extensions/chrome_v8_context.cc:80: v8::Context::Scope scope(v8_context()); On 2013/06/18 23:49:16, lazyboy wrote: > @kalman, did you mean here? How can I (or do I need to?) get rid of handle_scope > here? > > I'm still seeing the same behavior for document-s. Yep, thanks.
On 2013/06/18 22:54:47, lazyboy wrote: > And missed draft publishing... > > https://chromiumcodereview.appspot.com/16975007/diff/20001/chrome/renderer/re... > File chrome/renderer/resources/extensions/web_view.js (right): > > https://chromiumcodereview.appspot.com/16975007/diff/20001/chrome/renderer/re... > chrome/renderer/resources/extensions/web_view.js:52: if (documentA == documentB) > { > On 2013/06/18 22:26:49, adamk wrote: > > This only makes sense to me if documentA is undefined here. Is it? > > Surprisingly it is not, I tried to addEventListener to it and that didn't throw > any exception. > I just double checked now trying to window.console.log(documentA) and it prints > [object HTMLDocumentElement]. Do you mean HTMLDocument? Is the window different? Try isSameNode and see what that returns.
I would like to point out two fairly orthogonal issues: - running code on context startup is wasteful and makes it slower. if we only actually ran that webview shim when the document element is available, the whole listener thing would be moot. - similarly, it's a lot of code to be injecting into a context that might not have a webview (notably background pages probably don't have a webview). we should inject just the mutation observer and that is it, then when a webview tag appears run the injection code. - that said, if all this is mooted by the web component anyway - never mind :)
On 2013/06/19 01:00:02, kalman wrote: > On 2013/06/18 22:54:47, lazyboy wrote: > > And missed draft publishing... > > > > > https://chromiumcodereview.appspot.com/16975007/diff/20001/chrome/renderer/re... > > File chrome/renderer/resources/extensions/web_view.js (right): > > > > > https://chromiumcodereview.appspot.com/16975007/diff/20001/chrome/renderer/re... > > chrome/renderer/resources/extensions/web_view.js:52: if (documentA == > documentB) > > { > > On 2013/06/18 22:26:49, adamk wrote: > > > This only makes sense to me if documentA is undefined here. Is it? > > > > Surprisingly it is not, I tried to addEventListener to it and that didn't > throw > > any exception. > > I just double checked now trying to window.console.log(documentA) and it > prints > > [object HTMLDocumentElement]. > > Do you mean HTMLDocument? > > Is the window different? > > Try isSameNode and see what that returns. Sorry, yes it is HTMLDocument the window-s are the same. isSameNode returns same behavior (docAB = documentA.isSameNode(documentB), docBA = documentB.isSameNode(documentA)) (output from logs in patch set #6) (for background) [25240:25240:0619/145444:INFO:CONSOLE(53)] "old document: [object HTMLDocument]", source: webView (53) [25240:25240:0619/145444:INFO:CONSOLE(54)] "new document: [object HTMLDocument]", source: webView (54) [25240:25240:0619/145444:INFO:CONSOLE(56)] "both document are equal", source: webView (56) [25240:25240:0619/145444:INFO:CONSOLE(61)] "docAB: true", source: webView (61) [25240:25240:0619/145444:INFO:CONSOLE(63)] "docBA: true", source: webView (63) [25240:25240:0619/145444:INFO:CONSOLE(65)] "windowA == windowB", source: webView (65) [25240:25240:0619/145444:INFO:CONSOLE(72)] "shim: document.DOMContentLoaded", source: webView (72) (for app window) [25240:25240:0619/145445:INFO:CONSOLE(53)] "old document: [object HTMLDocument]", source: webView (53) [25240:25240:0619/145445:INFO:CONSOLE(54)] "new document: [object HTMLDocument]", source: webView (54) [25240:25240:0619/145445:INFO:CONSOLE(58)] "documents are different", source: webView (58) [25240:25240:0619/145445:INFO:CONSOLE(61)] "docAB: false", source: webView (61) [25240:25240:0619/145445:INFO:CONSOLE(63)] "docBA: false", source: webView (63) [25240:25240:0619/145445:INFO:CONSOLE(65)] "windowA == windowB", source: webView (65) [25240:25240:0619/145445:INFO:CONSOLE(72)] "shim: document.DOMContentLoaded", source: webView (72) [25240:25240:0619/145445:INFO:CONSOLE(1)] "L2. app document.DOMContentLoaded", source: chrome-extension://hcopflpnddjdbddphocgkfkhlpoclcgb/main.js (1) [25240:25240:0619/145445:INFO:CONSOLE(1)] "doTest", source: chrome-extension://hcopflpnddjdbddphocgkfkhlpoclcgb/main.js (1) [25240:25240:0619/145445:INFO:CONSOLE(1)] "webview element: [object HTMLUnknownElement]", source: chrome-extension://hcopflpnddjdbddphocgkfkhlpoclcgb/main.js (1) [25240:25240:0619/145445:INFO:CONSOLE(8)] "Uncaught TypeError: Cannot redefine property: name", source: chrome-extension://hcopflpnddjdbddphocgkfkhlpoclcgb/main.js (8) [25240:25240:0619/145445:INFO:CONSOLE(1)] "window.onload", source: chrome-extension://hcopflpnddjdbddphocgkfkhlpoclcgb/main.js (1)
On 2013/06/19 15:55:58, kalman wrote: > I would like to point out two fairly orthogonal issues: > - running code on context startup is wasteful and makes it slower. if we only > actually ran that webview shim when the document element is available, the whole > listener thing would be moot. > - similarly, it's a lot of code to be injecting into a context that might not > have a webview (notably background pages probably don't have a webview). we > should inject just the mutation observer and that is it, then when a webview tag > appears run the injection code. > - that said, if all this is mooted by the web component anyway - never mind :) Point noted along with Adam's point about MutationObserver, I will get back to them once we have a fix for this CL.
Instead of hooking DidCreateDocumentElement, I think you may be able to handle
this all in JS: see platform_app.js, which does this:
window.addEventListener('readystatechange', function(event) {
if (document.readyState != 'loading')
return;
// your code goes here
},
true); // capture
Note also the comment explaining why they do this (it's the same reason you're
doing what you're doing here):
// Document instance properties that we wish to disable need to be set when
// the document begins loading, since only then will the "document" reference
// point to the page's document (it will be reset between now and then).
// We can't listen for the "readystatechange" event on the document (because
// the object that it's dispatched on doesn't exist yet), but we can instead
// do it at the window level in the capturing phase.
On 2013/06/19 20:26:08, adamk wrote:
> Instead of hooking DidCreateDocumentElement, I think you may be able to handle
> this all in JS: see platform_app.js, which does this:
>
> window.addEventListener('readystatechange', function(event) {
> if (document.readyState != 'loading')
> return;
>
> // your code goes here
> },
> true); // capture
>
> Note also the comment explaining why they do this (it's the same reason you're
> doing what you're doing here):
>
> // Document instance properties that we wish to disable need to be set when
> // the document begins loading, since only then will the "document" reference
> // point to the page's document (it will be reset between now and then).
> // We can't listen for the "readystatechange" event on the document (because
> // the object that it's dispatched on doesn't exist yet), but we can instead
> // do it at the window level in the capturing phase.
Interesting! That worked, uploaded the change. (#8)
https://chromiumcodereview.appspot.com/16975007/diff/37002/chrome/renderer/re... File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://chromiumcodereview.appspot.com/16975007/diff/37002/chrome/renderer/re... chrome/renderer/resources/extensions/app_window_custom_bindings.js:61: renderViewObserverNatives.OnDocumentCreated(windowParams.viewId, Do you still need this change?
https://chromiumcodereview.appspot.com/16975007/diff/37002/chrome/renderer/re... File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://chromiumcodereview.appspot.com/16975007/diff/37002/chrome/renderer/re... chrome/renderer/resources/extensions/app_window_custom_bindings.js:61: renderViewObserverNatives.OnDocumentCreated(windowParams.viewId, On 2013/06/19 20:56:38, adamk wrote: > Do you still need this change? This is a separate change kalman@ asked me to do, I'll wait for him to comment and then revert or submit it as separate CL.
I'm not fussed. I guess we should move it back now that it's not shared anymore. It would be nice to maintain the fixup that you did there, but probably in a separate change.
On 2013/06/19 21:04:08, kalman wrote: > I'm not fussed. I guess we should move it back now that it's not shared anymore. > It would be nice to maintain the fixup that you did there, but probably in a > separate change. OK, I've removed the unrelated changes, this is very small now! Also added test. I will send out the other cleanups in separate CL.
lgtm But I hope you're not depending on this for security: any code run during page loaded (e.g., loaded from a <script> tag) could observe the WebView element before this script runs.
Also, please update CL description to match the actual fix
On 2013/06/19 22:04:32, adamk wrote: > Also, please update CL description to match the actual fix CL description updated, thanks a lot for thorough review! kalman@ can you lgtm for chrome/
https://chromiumcodereview.appspot.com/16975007/diff/14/chrome/test/data/exte... File chrome/test/data/extensions/platform_apps/web_view/document_ready/embedder.js (right): https://chromiumcodereview.appspot.com/16975007/diff/14/chrome/test/data/exte... chrome/test/data/extensions/platform_apps/web_view/document_ready/embedder.js:17: window.console.log('expected exception: ' + e); this seems like it will log on success. that is confusing. https://chromiumcodereview.appspot.com/16975007/diff/14/chrome/test/data/exte... chrome/test/data/extensions/platform_apps/web_view/document_ready/embedder.js:29: chrome.test.assertFalse(canRedefineNameProperty); why the need for the function above? could this be: function checkPartition() { try { Object.defineProperty(webview, 'name', {...}); chrome.test.fail('defineProperty should not work'); } catch(expected) {} chrome.test.assertEq('persist:test-partition', partitionName); } though besides which maybe it should be in a different test within runTests since it doesn't have anything to do with the partition.
https://chromiumcodereview.appspot.com/16975007/diff/14/chrome/test/data/exte... File chrome/test/data/extensions/platform_apps/web_view/document_ready/embedder.js (right): https://chromiumcodereview.appspot.com/16975007/diff/14/chrome/test/data/exte... chrome/test/data/extensions/platform_apps/web_view/document_ready/embedder.js:17: window.console.log('expected exception: ' + e); On 2013/06/19 23:02:54, kalman wrote: > this seems like it will log on success. that is confusing. Right, debug code, removed. https://chromiumcodereview.appspot.com/16975007/diff/14/chrome/test/data/exte... chrome/test/data/extensions/platform_apps/web_view/document_ready/embedder.js:29: chrome.test.assertFalse(canRedefineNameProperty); On 2013/06/19 23:02:54, kalman wrote: > why the need for the function above? could this be: > > function checkPartition() { > try { > Object.defineProperty(webview, 'name', {...}); > chrome.test.fail('defineProperty should not work'); > } catch(expected) {} > chrome.test.assertEq('persist:test-partition', partitionName); > } This would run the Object.defineProperty a bit later (webview.loadstop) than what we are trying to check (document.DOMContentLoaded). > > though besides which maybe it should be in a different test within runTests > since it doesn't have anything to do with the partition. That would require to entirely copy the test for the same reason as above, the tests for partition is run on webview.loadstop. I've renamed the test to clarify that we are doing two checks in the test.
lgtm https://chromiumcodereview.appspot.com/16975007/diff/52001/chrome/test/data/e... File chrome/test/data/extensions/platform_apps/web_view/document_ready/embedder.js (right): https://chromiumcodereview.appspot.com/16975007/diff/52001/chrome/test/data/e... chrome/test/data/extensions/platform_apps/web_view/document_ready/embedder.js:29: if (partitionName == 'persist:test-partition') { maybe you should still change this to be chrome.test.assertEq('persist:test-partition', partitionName) as a little cleanup.
https://chromiumcodereview.appspot.com/16975007/diff/52001/chrome/test/data/e... File chrome/test/data/extensions/platform_apps/web_view/document_ready/embedder.js (right): https://chromiumcodereview.appspot.com/16975007/diff/52001/chrome/test/data/e... chrome/test/data/extensions/platform_apps/web_view/document_ready/embedder.js:29: if (partitionName == 'persist:test-partition') { On 2013/06/20 02:51:27, kalman wrote: > maybe you should still change this to be > chrome.test.assertEq('persist:test-partition', partitionName) as a little > cleanup. Right, Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lazyboy@chromium.org/16975007/55001
Message was sent while issue was closed.
Change committed as 207423 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
