Closed Bug 887865 Opened 6 years ago Closed 5 years ago

Use a mozIStorageAsyncConnection in for GetIsVisitedStatement

Categories

(Toolkit :: Places, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
mozilla34
Iteration:
34.2

People

(Reporter: mak, Assigned: mak)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

History.cpp clones the Places connection on main-thread, since it's used for link coloring it may cause jank on first page load, so we should convert it to clone asynchronously.
Summary: Use an mozIStorageAsyncConnection in for GetIsVisitedStatement → Use a mozIStorageAsyncConnection in for GetIsVisitedStatement
Flags: firefox-backlog+
Iteration: --- → 34.2
Points: --- → 3
Iteration: 34.2 → ---
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Iteration: --- → 34.2
QA Whiteboard: [qa-]
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #8470029 - Flags: review?(mano)
the patch is mostly fine, but it's causing
Assertion failure: !mAsyncExecutionThread (AsyncClose has not been invoked on this connection!)

Since we asyncClose both connections I suspect there's something else involved, off-hand I can't tell if it's the main connection or the clone that causes it. Will need some debugger work.
Comment on attachment 8470029 [details] [diff] [review]
patch v1

Clearing request cause I must refactor this a little bit.

Basically the statement request can happen multiple times before the first request calls back, so I need a way to enqueue callbacks to avoid creating multiple clones.
Attachment #8470029 - Flags: review?(mano)
Attached patch patch v2 (obsolete) — Splinter Review
OK, I had to refactor all the isvisited statement handling, but I think the result is nicer.

This is currently running on tryserver to check I catched everything:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e4f9e331bf3f
Attachment #8470029 - Attachment is obsolete: true
Attachment #8471020 - Flags: review?(mano)
Comment on attachment 8471020 [details] [diff] [review]
patch v2

Review of attachment 8471020 [details] [diff] [review]:
-----------------------------------------------------------------

Just some nits.

::: toolkit/components/places/History.cpp
@@ +437,1 @@
>  public:

Move that under |public:|, and please add a new line between that to the next method.

@@ +479,5 @@
> +  NS_IMETHOD Complete(nsresult aResult, nsISupports* aStatement)
> +  {
> +    if (NS_FAILED(aResult)) {
> +      return aResult;
> +    }

It should be mentioned somewhere here that the return value of this method actually matters: that is, that it's not that just some XPCOM service calls this method, but that we call this method internally and check its return value.

You do an if (NS_FAILED..) check here, but few line later you use NS_ENSURE_SUCCESS. Any reason for that?

@@ +558,5 @@
>    , mIsVisited(aIsVisited)
>    {
>    }
>  
> +  ~VisitedQuery()

Either virtual, or MOZ_FINAL the class.

@@ +2032,5 @@
> +    DebugOnly<nsresult> rv = aDBConn->AsyncClone(true, this);
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));
> +  }
> +
> +  NS_IMETHODIMP Complete(nsresult aStatus, nsISupports* aConnection) {

NS_IMETHOD

@@ +2034,5 @@
> +  }
> +
> +  NS_IMETHODIMP Complete(nsresult aStatus, nsISupports* aConnection) {
> +    if (NS_FAILED(aStatus))
> +      return aStatus;

And here I guess it doesn't really matter, does it? If not, just return NS_OK.

@@ +2047,5 @@
> +      ),  getter_AddRefs(mIsVisitedStatement));
> +      MOZ_ASSERT(mIsVisitedStatement);
> +      for (int32_t i = 0; i < mIsVisitedCallbacks.Count(); ++i) {
> +        DebugOnly<nsresult> rv;
> +        if (mIsVisitedStatement) {

Let's do the check outside, along the assert, save the status and pass it to Complete, unconditionally.

@@ +2063,5 @@
> +
> +  void GetIsVisitedStatement(mozIStorageCompletionCallback* aCallback)
> +  {
> +    if (mIsVisitedStatement) {
> +      aCallback->Complete(NS_OK, mIsVisitedStatement);

(void)
Attachment #8471020 - Flags: review?(mano) → review+
Attached patch patch v3Splinter Review
Addressed comments.
Attachment #8471020 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/b3877ffb2c4a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.