Tell the chrome process when we visit a page

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: fred23, Assigned: dougt)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

9 years ago
Remote AddVisit() calls from content to chrome process so that it can add a visit to the history database properly.
(Reporter)

Comment 1

9 years ago
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().
(Reporter)

Comment 2

9 years ago
Created attachment 431713 [details] [diff] [review]
patch v.1

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.
(Reporter)

Comment 4

9 years ago
Created attachment 431735 [details] [diff] [review]
patch v.2

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.
(Reporter)

Comment 7

9 years ago
Created attachment 431897 [details] [diff] [review]
patch v.3

(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).
(Reporter)

Comment 10

9 years ago
(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.
(Reporter)

Comment 11

9 years ago
Created attachment 432647 [details] [diff] [review]
patch v.4

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)
(Reporter)

Comment 12

9 years ago
Created attachment 434279 [details] [diff] [review]
patch v.5

Fixed bitrot.
Attachment #432647 - Attachment is obsolete: true
Attachment #434279 - Flags: review?(bzbarsky)
Attachment #432647 - Flags: review?(bzbarsky)
(Reporter)

Comment 13

9 years ago
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
(Assignee)

Comment 14

9 years ago
Created attachment 446030 [details] [diff] [review]
patch v.6
Attachment #434279 - Attachment is obsolete: true
Attachment #446030 - Flags: review?(sdwilsh)
(Assignee)

Updated

9 years ago
Assignee: bugzillaFred → dougt
>+NS_ASSERTION(!(aReferrer && aRedirectFrom), "Both can not be non-null");

That message is needlessly complicated to parse.
(Assignee)

Updated

9 years ago
Attachment #446030 - Attachment is obsolete: true
Attachment #446030 - Flags: review?(sdwilsh)
(Assignee)

Comment 16

9 years ago
the patch is obsolete for ^ and other reasons. ;-)

Updated

9 years ago
Depends on: 566799
(Assignee)

Comment 17

9 years ago
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)

Updated

9 years ago
Attachment #446030 - Flags: review?(sdwilsh) → review-
Comment on attachment 446030 [details] [diff] [review]
patch v.6

per in-person discussion with dougt
(Assignee)

Comment 19

9 years ago
Created attachment 447634 [details] [diff] [review]
patch
Attachment #446030 - Attachment is obsolete: true
Attachment #447634 - Flags: review?(sdwilsh)
(Assignee)

Updated

9 years ago
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+
(Assignee)

Updated

8 years ago
Attachment #447634 - Flags: superreview?(bzbarsky)
Comment on attachment 447634 [details] [diff] [review]
patch

r=sdwilsh on the places changes
Attachment #447634 - Flags: review+
(Assignee)

Comment 22

8 years ago
http://hg.mozilla.org/projects/electrolysis/rev/71114e4522c7
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Attachment #447634 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 23

8 years ago
this was backed out a few days ago because the required places patches were backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 457431 [details] [diff] [review]
patch v1.1

just the patch Doug pushed to tryserver, that applies.
Attachment #447634 - Attachment is obsolete: true
(Assignee)

Comment 25

8 years ago
http://hg.mozilla.org/mozilla-central/rev/da160d0117e7
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.