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

Issue 18467003: Refactoring: Introduce HTMLImport interface to form import tree. (Closed)

Created:
7 years, 5 months ago by Hajime Morrita
Modified:
7 years, 5 months ago
CC:
blink-reviews, dglazkov+blink, eae+blinkwatch, adamk+blink_chromium.org
Visibility:
Public.

Description

Refactoring: Introduce HTMLImport interface to form import tree. This change introduces HTMLImport abstract class which is derived by HTMLImportLoader and HTMLImportsController. - A set of HTMLImport forms a tree which represents the import dependnecy. Currently only child-to-parent link is available. The tree root is a HTMLImportController. - Document now refers HTMLImport instead of HTMLImportController. For the master document, the object is HTMLImportController. For each imported document, it is a HTMLImportLoader which loads the document. Note that the only interaction from a document to its import is to ask if all of its (sub) imports are loaded. - HTMLImportsController is turned from a RefCounted to a document Supplement. The lifetime is managed by the supplement machinery instead of RefPtr which Document::m_import originally was. Now m_import is a weak, raw pointer and is cleared by HTMLImport subclasses. - didLoad() notification is now fired recursively up to the import dependency chain. This is crusial when scripts inside imported documents block the document parsing and let the notification unblock it. TEST=none BUG=240592 R=dglazkov@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153977 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154082

Patch Set 1 #

Total comments: 1

Patch Set 2 : For landing #

Total comments: 3

Patch Set 3 : Addressed comments, landing. #

Patch Set 4 : Added missing checks and a clearance. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+149 lines, -65 lines) Patch
M Source/core/core.gypi View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/dom/Document.h View 1 2 3 3 chunks +4 lines, -4 lines 0 comments Download
M Source/core/dom/Document.cpp View 1 2 3 6 chunks +16 lines, -6 lines 0 comments Download
A + Source/core/html/HTMLImport.h View 1 2 3 1 chunk +13 lines, -14 lines 0 comments Download
A + Source/core/html/HTMLImport.cpp View 1 chunk +7 lines, -6 lines 0 comments Download
M Source/core/html/HTMLImportsController.h View 1 2 3 6 chunks +25 lines, -10 lines 0 comments Download
M Source/core/html/HTMLImportsController.cpp View 1 2 3 9 chunks +82 lines, -25 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
Hajime Morrita
Could you take a look?
7 years, 5 months ago (2013-07-10 09:02:30 UTC) #1
dglazkov
lgtm https://codereview.chromium.org/18467003/diff/1/Source/core/html/HTMLImportsController.cpp File Source/core/html/HTMLImportsController.cpp (right): https://codereview.chromium.org/18467003/diff/1/Source/core/html/HTMLImportsController.cpp#newcode261 Source/core/html/HTMLImportsController.cpp:261: for (HTMLImport* notifiee = loadedImport->parent(); notifiee; notifiee = ...
7 years, 5 months ago (2013-07-10 17:54:22 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/morrita@chromium.org/18467003/5001
7 years, 5 months ago (2013-07-11 00:23:34 UTC) #3
commit-bot: I haz the power
Retried try job too often on mac_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout_rel&number=12295
7 years, 5 months ago (2013-07-11 02:05:22 UTC) #4
dominicc (has gone to gerrit)
Here are some posthumous comments. They're late--feel free to treat them as FYI. It is ...
7 years, 5 months ago (2013-07-11 03:26:48 UTC) #5
Hajime Morrita
Thanks for the review, Dominic! I'll update the patch. I hoped the last one to ...
7 years, 5 months ago (2013-07-11 04:56:02 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/morrita@chromium.org/18467003/27001
7 years, 5 months ago (2013-07-11 05:01:42 UTC) #7
commit-bot: I haz the power
Retried try job too often on linux_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layout_rel&number=16072
7 years, 5 months ago (2013-07-11 05:39:44 UTC) #8
Hajime Morrita
Committed patchset #3 manually as r153977 (presubmit successful).
7 years, 5 months ago (2013-07-11 05:54:04 UTC) #9
Hajime Morrita
Reverted because of crashes: STDOUT: #CRASHED - renderer (pid 6299) STDERR: Received signal 11 SEGV_MAPERR ...
7 years, 5 months ago (2013-07-11 09:30:48 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/morrita@chromium.org/18467003/40001
7 years, 5 months ago (2013-07-12 01:11:36 UTC) #11
commit-bot: I haz the power
7 years, 5 months ago (2013-07-12 05:09:14 UTC) #12
Message was sent while issue was closed.
Change committed as 154082

Powered by Google App Engine
This is Rietveld 408576698