Closed Bug 551181 Opened 12 years ago Closed 12 years ago

Tell the chrome process when we visit a page

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: fred23, Assigned: dougt)

References

Details

Attachments

(1 file, 7 obsolete files)

Remote AddVisit() calls from content to chrome process so that it can add a visit to the history database properly.
Well, in fact, the entry point to adding a new Visit from content is really through nsDocShell::AddToGlobalhistory(...), which is consumed by many nsDocShell foos like : ::OnNewURI(), ::OnRedirectStateChange() and ::AddState().
Attached patch patch v.1 (obsolete) — Splinter Review
This is "part2" of the remote link-visited issue in e10s.
By applying this patch *on top* of patch for bug 516728 (part1), link coloring in e10s becomes operational.

I still dislike all the ""// reconstruct our IPDL-passed URIs"" parts, but there's a bug already filed (Bug 541174) for passing nsIURI across process boundary in a more optimized way.
Attachment #431713 - Flags: review?(bzbarsky)
A few quick notes:

1)  No need to assign "" to aReferrerSpec.  It's already empty.
2)  Please don't use the aFoo naming convention for things that aren't arguments.
3)  In MOZ_IPC builds in the content process, we'll notify the content-side
    mGlobalHistory too.  Is that what we want?
4)  Why are we overriding the failing rv in RecvIsVisited?
5)  If we want to check whether aReferrerSpec is empty, just use IsEmpty() on the
    string instead of EqualsLiteral(""), please.
Attached patch patch v.2 (obsolete) — Splinter Review
Patch corrected per Boris's comments.

(In reply to comment #3)
> 3)  In MOZ_IPC builds in the content process, we'll notify the content-side
>     mGlobalHistory too.  Is that what we want?

I'm not sure I understand what you mean here. really, the content-side mGlobalHistory is 0x00000000 (not accessible from content)... Tell me more, it's possible I've overlooked something here. thanks.
Attachment #431713 - Attachment is obsolete: true
Attachment #431735 - Flags: review?(bzbarsky)
Attachment #431713 - Flags: review?(bzbarsky)
Oh, I see.  Your code just makes it so that visiting a link in the chrome process doesn't add it to history.  Is that desired?  I guess we do that right now for type="chrome" docshells, but I believe the current e10s setup is that you can have type="content" docshells in the chrome process...
Also, as currently written ifdef MOZ_IPC and if we're not in the content process then visited never gets set and we branch on a random value.
Attached patch patch v.3 (obsolete) — Splinter Review
(In reply to comment #6)
> Also, as currently written ifdef MOZ_IPC and if we're not in the content
> process then visited never gets set and we branch on a random value.

Yeah, you're right! I just didn't think at first that the chrome process could use this codepath. Here's an updated patch. This version induces a tiny code redundancy between the MOZ_IPC and non-MOZ_IPC cases, but I prefer that to putting that code in a new function.
Attachment #431735 - Attachment is obsolete: true
Attachment #431897 - Flags: review?(bzbarsky)
Attachment #431735 - Flags: review?(bzbarsky)
> This version induces a tiny code redundancy 

Why, though?  This should work fine:

#ifdef MOZ_IPC
  if (XRE_GetProcessType() == GeckoProcessType_Content) {
  }
  else
#endif /* MOZ_IPC */
  if (mGlobalHistory) {
    // Code for chrome-process stuff and non-moz-ipc stuff here.
  }

and wouldn't require that initial ifdefed !mGlobalHistory check if you also condition the notification below that on mGlobalHistory.
One other nit, while I'm here.  Sentence-like comments need capitalization and punctuation (and use of ';', not ',', as appropriate).
(In reply to comment #8)
> Why, though?  This should work fine:
> 
> #ifdef MOZ_IPC
>   if (XRE_GetProcessType() == GeckoProcessType_Content) {
>   }
>   else
> #endif /* MOZ_IPC */
>   if (mGlobalHistory) {
>     // Code for chrome-process stuff and non-moz-ipc stuff here.
>   }

Well, I had considered this option, but I personally don't like it because having "if()...else" blocks cross #ifdef boundaries break indentation levels and make the code harder to read, but as I see it's allright with you, I can update the patch accordingly, since it has indeed some interesting advantages, like removing code duplication.

> and wouldn't require that initial ifdefed !mGlobalHistory check if you also
> condition the notification below that on mGlobalHistory.

true.
Attached patch patch v.4 (obsolete) — Splinter Review
Here's patch v.4, per bz's comments. thanks.
Attachment #431897 - Attachment is obsolete: true
Attachment #432647 - Flags: review?(bzbarsky)
Attachment #431897 - Flags: review?(bzbarsky)
Attached patch patch v.5 (obsolete) — Splinter Review
Fixed bitrot.
Attachment #432647 - Attachment is obsolete: true
Attachment #434279 - Flags: review?(bzbarsky)
Attachment #432647 - Flags: review?(bzbarsky)
Comment on attachment 434279 [details] [diff] [review]
patch v.5

Removing the review request for now, as this bug is going to be affected by Stover's work (bug 556400). When his work posts/lands, I'll retouch this bug and ask for a review.
Attachment #434279 - Flags: review?(bzbarsky)
Assignee: bugzillaFred → webapps
Assignee: webapps → bugzillaFred
Attached patch patch v.6 (obsolete) — Splinter Review
Attachment #434279 - Attachment is obsolete: true
Attachment #446030 - Flags: review?(sdwilsh)
Assignee: bugzillaFred → dougt
>+NS_ASSERTION(!(aReferrer && aRedirectFrom), "Both can not be non-null");

That message is needlessly complicated to parse.
Attachment #446030 - Attachment is obsolete: true
Attachment #446030 - Flags: review?(sdwilsh)
the patch is obsolete for ^ and other reasons. ;-)
Depends on: 566799
Comment on attachment 446030 [details] [diff] [review]
patch v.6

lets review this and ignore that assertion.
Attachment #446030 - Attachment is obsolete: false
Attachment #446030 - Flags: review?(sdwilsh)
Attachment #446030 - Flags: review?(sdwilsh) → review-
Comment on attachment 446030 [details] [diff] [review]
patch v.6

per in-person discussion with dougt
Attached patch patch (obsolete) — Splinter Review
Attachment #446030 - Attachment is obsolete: true
Attachment #447634 - Flags: review?(sdwilsh)
Attachment #447634 - Flags: review?(sdwilsh) → review?(jones.chris.g)
Comment on attachment 447634 [details] [diff] [review]
patch

>diff --git a/dom/ipc/ContentProcessParent.cpp b/dom/ipc/ContentProcessParent.cpp
>--- a/dom/ipc/ContentProcessParent.cpp
>+++ b/dom/ipc/ContentProcessParent.cpp
>@@ -41,17 +41,17 @@
> 
> #include "TabParent.h"
> #include "mozilla/ipc/TestShellParent.h"
> #include "mozilla/net/NeckoParent.h"
> #include "nsIPrefBranch.h"
> #include "nsIPrefBranch2.h"
> #include "nsIPrefLocalizedString.h"
> #include "nsIObserverService.h"
>-
>+#include "nsContentUtils.h"
> #include "nsAutoPtr.h"
> #include "nsCOMPtr.h"
> #include "nsServiceManagerUtils.h"
> #include "nsThreadUtils.h"
> #include "nsChromeRegistryChrome.h"
> 
> using namespace mozilla::ipc;
> using namespace mozilla::net;
>@@ -373,16 +373,29 @@ ContentProcessParent::RequestRunToComple
>         printf("Running to completion...\n");
> #endif
>         mRunToCompletionDepth = 1;
>         mShouldCallUnblockChild = true;
>     }
>     return !!mRunToCompletionDepth;
> }
> 
>+
>+bool
>+ContentProcessParent::RecvVisitURI(const IPC::URI& uri,
>+                                   const IPC::URI& referrer,
>+                                   const PRUint32& flags)
>+{
>+    nsCOMPtr<nsIURI> ourURI = uri;
>+    nsCOMPtr<nsIURI> ourReferrer = referrer;
>+    IHistory *history = nsContentUtils::GetHistory(); 
>+    history->VisitURI(ourURI, referrer, flags);

I would have expected a NULL-check here but I don't know that the
GetHistory() API contract is (looks to be in a pending patch).

>diff --git a/dom/ipc/PContentProcess.ipdl b/dom/ipc/PContentProcess.ipdl
>--- a/dom/ipc/PContentProcess.ipdl
>+++ b/dom/ipc/PContentProcess.ipdl
>@@ -37,17 +37,19 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> include protocol PIFrameEmbedding;
> include protocol PTestShell;
> include protocol PNecko;
> 
> include "mozilla/TabTypes.h";
> include "mozilla/chrome/RegistryMessageUtils.h";
>+include "mozilla/net/NeckoMessageUtils.h";
> 
>+using IPC::URI;
> using ChromePackage;
> using ResourceMapping;
> using OverrideMapping;
> 
> namespace mozilla {
> namespace dom {
> 
> rpc protocol PContentProcess
>@@ -66,16 +68,18 @@ child:
> 
>     async SetOffline(PRBool offline);
> 
>     NotifyRemotePrefObserver(nsCString aDomain);
> 
> parent:
>     PNecko();
> 
>+    async VisitURI(URI uri, URI referrer, PRUint32 flags);
>+

Is it worth validating flags sent by the content process?  Can it
cause mischief by sending bad flags?  (Looks like the patch that adds
the flags hasn't landed yet so I don't know what they are.)

>diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp
>--- a/toolkit/components/places/src/History.cpp
>+++ b/toolkit/components/places/src/History.cpp
>@@ -32,16 +32,21 @@
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>+#ifdef MOZ_IPC
>+#include "mozilla/dom/ContentProcessChild.h"
>+#include "nsXULAppAPI.h"
>+#endif
>+
> #include "History.h"
> #include "nsNavHistory.h"
> #include "nsNavBookmarks.h"
> #include "Helpers.h"
> 
> #include "mozilla/storage.h"
> #include "mozilla/dom/Link.h"
> #include "nsDocShellCID.h"
>@@ -789,16 +794,26 @@ History::StartNextVisitURI()
> 
> NS_IMETHODIMP
> History::VisitURI(nsIURI* aURI,
>                   nsIURI* aLastVisitedURI,
>                   PRUint32 aFlags)
> {
>   NS_PRECONDITION(aURI, "URI should not be NULL.");
> 
>+#ifdef MOZ_IPC
>+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+    mozilla::dom::ContentProcessChild * cpc = 
>+      mozilla::dom::ContentProcessChild::GetSingleton();

|using mozilla::dom::ContentProcessChild| please.
Attachment #447634 - Flags: review?(jones.chris.g) → review+
Attachment #447634 - Flags: superreview?(bzbarsky)
Comment on attachment 447634 [details] [diff] [review]
patch

r=sdwilsh on the places changes
Attachment #447634 - Flags: review+
http://hg.mozilla.org/projects/electrolysis/rev/71114e4522c7
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #447634 - Flags: superreview?(bzbarsky) → superreview+
this was backed out a few days ago because the required places patches were backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch v1.1Splinter Review
just the patch Doug pushed to tryserver, that applies.
Attachment #447634 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/da160d0117e7
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.