Closed
Bug 1102060
Opened 10 years ago
Closed 10 years ago
PScreenManager::ScreenForBrowser doesn't work in nested content process
Categories
(Core :: DOM: Content Processes, defect)
Core
DOM: Content Processes
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: kershaw, Assigned: chunmin)
References
Details
Attachments
(2 files, 13 obsolete files)
5.25 KB,
patch
|
chunmin
:
review+
|
Details | Diff | Splinter Review |
5.04 KB,
patch
|
chunmin
:
review+
|
Details | Diff | Splinter Review |
prio(high) sync ScreenForBrowser(PBrowser aBrowser) returns (ScreenDetails screen, bool success); Since the TabParent may live in content process now, PBrowser may have to change to PBrowserOrId for above IPC.
Reporter | ||
Updated•10 years ago
|
Blocks: nested-oop
Reporter | ||
Updated•10 years ago
|
Blocks: nested-oop-tests
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → cchang
QA Contact: cchang
Reporter | ||
Comment 2•10 years ago
|
||
Comment on attachment 8545106 [details] [diff] [review] bug-1102060-fix.patch Review of attachment 8545106 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks ok, but you have to watch out those nits. ::: dom/ipc/ContentProcessManager.cpp @@ +280,5 @@ > +ContentProcessManager::GetTabParentByProcessAndTabId(const ContentParentId& aChildCpId, > + const TabId& aChildTabId) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + nit: trailing space @@ +286,5 @@ > + if (NS_WARN_IF(iter == mContentParentMap.end())) { > + ASSERT_UNLESS_FUZZING(); > + return nullptr; > + } > + nit: trailing space ::: dom/ipc/ContentProcessManager.h @@ +108,5 @@ > */ > bool GetRemoteFrameOpenerTabId(const ContentParentId& aChildCpId, > const TabId& aChildTabId, > /*out*/ TabId* aOpenerTabId); > + nit: trailing space @@ +115,5 @@ > + */ > + already_AddRefed<TabParent> > + GetTabParentByProcessAndTabId(const ContentParentId& aChildCpId, > + const TabId& aChildTabId); > + nit: trailing space ::: dom/ipc/ScreenManagerParent.cpp @@ +130,5 @@ > > // Find the mWidget associated with the tabparent, and then return > // the nsIScreen it's on. > + ContentParent* cp = static_cast<ContentParent*>(this->Manager()); > + nsRefPtr<TabParent> tabParent = GetTopLevelTabParentByProcessAndTabId(cp->ChildID(), aTabId); This line is too long. @@ +169,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + ContentProcessManager* cpm = ContentProcessManager::GetSingleton(); > + nit: trailing space @@ +171,5 @@ > + > + ContentProcessManager* cpm = ContentProcessManager::GetSingleton(); > + > + // To get the upper ContentParentId by the current ContentParentId > + ContentParentId upperCpId; parentCpId is better @@ +172,5 @@ > + ContentProcessManager* cpm = ContentProcessManager::GetSingleton(); > + > + // To get the upper ContentParentId by the current ContentParentId > + ContentParentId upperCpId; > + nit: trailing space @@ +182,5 @@ > + if(upperCpId == 0){ > + // This content parent is already on top level > + return cpm->GetTabParentByProcessAndTabId(aChildCpId, aChildTabId); > + } > + nit: trailing space @@ +184,5 @@ > + return cpm->GetTabParentByProcessAndTabId(aChildCpId, aChildTabId); > + } > + > + // To get the upper TabParentId by the current ContentParentId and TabId > + TabId upperTabId; openerTabId ::: dom/ipc/ScreenManagerParent.h @@ +45,5 @@ > ScreenDetails* aRetVal, > bool* aSuccess); > > private: > + /** nit: trailing space @@ +50,5 @@ > + * Get the TabParent on top level by the given content process and tab id. > + */ > + already_AddRefed<TabParent> > + GetTopLevelTabParentByProcessAndTabId(const ContentParentId& aChildCpId, > + const TabId& aChildTabId); I am thinking to move this function to ContentProcessManager.h, because bug 1020179 also needs to get the top level TabParent. I will ask reviewer's thought and let you know. @@ +51,5 @@ > + */ > + already_AddRefed<TabParent> > + GetTopLevelTabParentByProcessAndTabId(const ContentParentId& aChildCpId, > + const TabId& aChildTabId); > + nit: trailing space
Attachment #8545106 -
Flags: feedback?(kechang)
Assignee | ||
Comment 3•10 years ago
|
||
Rename some variables in ScreenManagerParent.cpp and clear the nits
Assignee | ||
Comment 4•10 years ago
|
||
Rename the variables and clear the nits
Attachment #8547339 -
Attachment is obsolete: true
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8545106 -
Attachment is obsolete: true
Attachment #8547387 -
Attachment is obsolete: true
Attachment #8549326 -
Flags: feedback?(kechang)
Reporter | ||
Comment 7•10 years ago
|
||
Comment on attachment 8549326 [details] [diff] [review] Part1: Add new function to get TabParent in ContentProcessManager Review of attachment 8549326 [details] [diff] [review]: ----------------------------------------------------------------- Please also move GetTopLevelTabParentByProcessAndTabId into ContentProcessManager so bug 1020179 can also reuse this function. Thanks. ::: dom/ipc/ContentProcessManager.cpp @@ +275,5 @@ > > return true; > } > > +already_AddRefed<TabParent> You have to include TabParent.h in this cpp. @@ +288,5 @@ > + return nullptr; > + } > + > + const InfallibleTArray<PBrowserParent*>& browsers = > + iter->second.mCp->ManagedPBrowserParent(); nit: add two spaces indent here
Attachment #8549326 -
Flags: feedback?(kechang) → feedback+
Reporter | ||
Comment 8•10 years ago
|
||
Comment on attachment 8549332 [details] [diff] [review] Part2: Change the way to send/rcv for ScreenForBrowser and add new function to get top level TabParent in ScreenManagerParent Review of attachment 8549332 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ScreenManagerParent.cpp @@ +189,5 @@ > + currentTabId = openerTabId; > + > + // Get the ContentParentId and TabId on upper level > + if(!cpm->GetParentProcessId(currentCpId, &parentCpId) > + || !cpm->GetRemoteFrameOpenerTabId(currentCpId, currentTabId, &openerTabId)){ nit: !cpm->GetParentProcessId(...) || !cpm->GetRemoteFrameOpenerTabId(...)
Attachment #8549332 -
Flags: feedback?(kechang) → feedback+
Reporter | ||
Comment 9•10 years ago
|
||
(In reply to Kershaw Chang [:kershaw] from comment #8) > Comment on attachment 8549332 [details] [diff] [review] > Part2: Change the way to send/rcv for ScreenForBrowser and add new function > to get top level TabParent in ScreenManagerParent > > Review of attachment 8549332 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/ipc/ScreenManagerParent.cpp > @@ +189,5 @@ > > + currentTabId = openerTabId; > > + > > + // Get the ContentParentId and TabId on upper level > > + if(!cpm->GetParentProcessId(currentCpId, &parentCpId) > > + || !cpm->GetRemoteFrameOpenerTabId(currentCpId, currentTabId, &openerTabId)){ > > nit: !cpm->GetParentProcessId(...) || > !cpm->GetRemoteFrameOpenerTabId(...) Remember to make these two conditions aligned.
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8549326 -
Attachment is obsolete: true
Attachment #8550042 -
Flags: feedback?(kechang)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8550042 -
Attachment is obsolete: true
Attachment #8550043 -
Attachment is obsolete: true
Attachment #8550042 -
Flags: feedback?(kechang)
Reporter | ||
Updated•10 years ago
|
Attachment #8550051 -
Flags: feedback?(kechang) → feedback+
Reporter | ||
Updated•10 years ago
|
Attachment #8550050 -
Flags: review?(khuey)
Reporter | ||
Comment 14•10 years ago
|
||
Hi Kyle, Could you help to review the part 1 patch? The function GetTabParentByProcessAndTabId is actually the same as the patch 1 in bug 1020179. We add another function GetTopLevelTabParentByProcessAndTabId to get the top level TabParent lived in b2g process. Thanks.
Reporter | ||
Comment 15•10 years ago
|
||
@chunmin, You may need to rebase the part 2 patch since Bug 1113006 is landed.
Assignee | ||
Comment 16•10 years ago
|
||
@Kershaw: Thank you for the reminder :)
Attachment #8550051 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8552135 -
Flags: review?(mconley)
Assignee | ||
Comment 17•10 years ago
|
||
Hi, Mike Could you have time to review the patch part2? I will appreciate your help.
Comment on attachment 8550050 [details] [diff] [review] Part1: Add two functions to get TabParent in ContentProcessManager by given ContentParentId and TabId I'm on vacation for two weeks, so 302 kanru.
Attachment #8550050 -
Flags: review?(khuey) → review?(kchen)
Comment 19•10 years ago
|
||
Comment on attachment 8550050 [details] [diff] [review] Part1: Add two functions to get TabParent in ContentProcessManager by given ContentParentId and TabId Review of attachment 8550050 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good. I want to take a second look with the styles nits fixed. ::: dom/ipc/ContentProcessManager.cpp @@ +305,5 @@ > +ContentProcessManager::GetTopLevelTabParentByProcessAndTabId(const ContentParentId& aChildCpId, > + const TabId& aChildTabId) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + nit: trailing whitespace @@ +317,5 @@ > + TabId openerTabId = aChildTabId; > + > + // Stop this loop when the upper ContentParentId of > + // the current ContentParentId is chrome(ContentParentId = 0). > + do{ nit: insert a space between do and { @@ +324,5 @@ > + currentTabId = openerTabId; > + > + // Get the ContentParentId and TabId on upper level > + if(!GetParentProcessId(currentCpId, &parentCpId) || > + !GetRemoteFrameOpenerTabId(currentCpId, currentTabId, &openerTabId)){ nit: trailing whitespace, insert a space between if and (, ) and { @@ +327,5 @@ > + if(!GetParentProcessId(currentCpId, &parentCpId) || > + !GetRemoteFrameOpenerTabId(currentCpId, currentTabId, &openerTabId)){ > + return nullptr; > + } > + }while(parentCpId); nit: insert a space between }, while and ( ::: dom/ipc/ContentProcessManager.h @@ +111,5 @@ > /*out*/ TabId* aOpenerTabId); > > + /** > + * Get the TabParent by the given content process and tab id. > + */ document that this function returns nullptr when TabParent couldn't be found via aChildTabId (probably because the TabParent is not in the chrome process) @@ +118,5 @@ > + const TabId& aChildTabId); > + > + /** > + * Get the TabParent on top level by the given content process and tab id. > + */ Maybe you could explain a bit more about what is a top level TabParent, its relation to the aChildTabId.
Attachment #8550050 -
Flags: review?(kchen) → feedback+
Comment 20•10 years ago
|
||
Comment on attachment 8552135 [details] [diff] [review] Bug 1102060 - Part2: Change the way to send/rcv for ScreenForBrowser Review of attachment 8552135 [details] [diff] [review]: ----------------------------------------------------------------- This looks OK to me, but you might want another reviewer, since I'm only about 50% confident with this stuff.
Attachment #8552135 -
Flags: review?(mconley)
Attachment #8552135 -
Flags: review?(bugs)
Attachment #8552135 -
Flags: feedback+
Comment 21•10 years ago
|
||
Comment on attachment 8552135 [details] [diff] [review] Bug 1102060 - Part2: Change the way to send/rcv for ScreenForBrowser >- TabParent* tabParent = static_cast<TabParent*>(aBrowser); >+ ContentParent* cp = static_cast<ContentParent*>(this->Manager()); >+ ContentProcessManager* cpm = ContentProcessManager::GetSingleton(); >+ nsRefPtr<TabParent> tabParent = >+ cpm->GetTopLevelTabParentByProcessAndTabId(cp->ChildID(), aTabId); (random note, our naming for parent/childID is so inconsistent :/ )
Attachment #8552135 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 22•10 years ago
|
||
Hi, Kanru Thanks for your help. I add a little more comments about top level TabParent. However, I assume the target audience already has the knowledge of IPC and nested oop. If the comment isn't clear enough, I could write some more. Could you give me some suggestions?
Attachment #8550050 -
Attachment is obsolete: true
Comment 24•10 years ago
|
||
Comment on attachment 8555057 [details] [diff] [review] Part1: Add two new functions to get TabParent in ContentProcessManager by given ContentParentId and TabId Review of attachment 8555057 [details] [diff] [review]: ----------------------------------------------------------------- The comment is much clearer now, thanks! ::: dom/ipc/ContentProcessManager.cpp @@ +320,5 @@ > + currentCpId = parentCpId; > + currentTabId = openerTabId; > + > + // Get the ContentParentId and TabId on upper level > + if(!GetParentProcessId(currentCpId, &parentCpId) || nit: insert a space between if and (
Attachment #8555057 -
Flags: review+
Assignee | ||
Comment 25•10 years ago
|
||
Hi, Kanru, I must thank you for your patient reviewing. I correct the style issue and rewrite the comment of GetTopLevelTabParentByProcessAndTabId again.
Attachment #8555057 -
Attachment is obsolete: true
Comment 26•10 years ago
|
||
Comment on attachment 8555745 [details] [diff] [review] [v4]Part1: Add two new functions to get TabParent in ContentProcessManager by given ContentParentId and TabId Review of attachment 8555745 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentProcessManager.cpp @@ +321,5 @@ > + currentTabId = openerTabId; > + > + // Get the ContentParentId and TabId on upper level > + if (!GetParentProcessId(currentCpId, &parentCpId) || > + !GetRemoteFrameOpenerTabId(currentCpId, currentTabId, &openerTabId)) { nit: indent
Attachment #8555745 -
Flags: review+
Assignee | ||
Comment 28•10 years ago
|
||
From try server test, there is some timeout issues: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f7113e41292
Comment 29•10 years ago
|
||
(In reply to Chun-Min Chang[:chunmin] from comment #28) > From try server test, there is some timeout issues: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f7113e41292 I re-triggered the tests, they looks green now.
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8556359 -
Attachment is obsolete: true
Attachment #8558367 -
Flags: review+
Assignee | ||
Comment 31•10 years ago
|
||
Attachment #8552135 -
Attachment is obsolete: true
Attachment #8558368 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 32•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd8292a2ed7b https://hg.mozilla.org/integration/mozilla-inbound/rev/e66dca57463b
Keywords: checkin-needed
Comment 33•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fd8292a2ed7b https://hg.mozilla.org/mozilla-central/rev/e66dca57463b
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•