Retrieve article original URL from entryID

Retrieve article original URL from entryID

This CL adds support for retrieving an article's original
URL based on it's entry ID.
Adds functions HasEntry and GetUrlForEntry for the
DomDistillerServiceInterface.
HasEntry returns whether DomDistillerService contains an
article for the corresponding entry ID.
GetUrlForEntry returns the original URL for a given entry ID.
Adds Java implementation to retrieve information about entry
through added native methods.

BUG=383630
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289761

[nyquist, 2014-08-13 20:09:11]

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_service.cc (right):

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_service.cc:74: // Returns the source
URL given an entry ID. If the entry ID article has
Move this comment to the interface and add a comment for HasEntry as well.

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_service.h:115: bool HasEntry(const
std::string entry_id);
Also add to DomDistillerServiceInterface. Also add the comments there. This
probably means you have to update the lazy service too.

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_service_unittest.cc (right):

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_service_unittest.cc:512: GURL
formattedURL(service_->GetUrlForEntry(entry_id));
Nit: This URL isn't really formatted in any special way, is it? Same in the next
test.

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
File components/dom_distiller/core/url_utils.cc (right):

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
components/dom_distiller/core/url_utils.cc:38: net::GetValueForKeyInQuery(url,
key, &value);
could you guard this with url.is_valid()? Just bail out and return the same as
not found in that case.

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
File components/dom_distiller/core/url_utils.h (right):

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
components/dom_distiller/core/url_utils.h:24: // Returns the value of the query
parameter for a given URL.
Add "value for the given |key|" after "query parameter"?

[sunangel, 2014-08-13 21:49:33]

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_service.cc (right):

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_service.cc:74: // Returns the source
URL given an entry ID. If the entry ID article has
On 2014/08/13 20:09:10, nyquist wrote:
> Move this comment to the interface and add a comment for HasEntry as well.

Done.

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_service.h:115: bool HasEntry(const
std::string entry_id);
On 2014/08/13 20:09:10, nyquist wrote:
> Also add to DomDistillerServiceInterface. Also add the comments there. This
> probably means you have to update the lazy service too.

Done.

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
File components/dom_distiller/core/dom_distiller_service_unittest.cc (right):

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
components/dom_distiller/core/dom_distiller_service_unittest.cc:512: GURL
formattedURL(service_->GetUrlForEntry(entry_id));
On 2014/08/13 20:09:11, nyquist wrote:
> Nit: This URL isn't really formatted in any special way, is it? Same in the
next
> test.

Done.

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
File components/dom_distiller/core/url_utils.cc (right):

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
components/dom_distiller/core/url_utils.cc:38: net::GetValueForKeyInQuery(url,
key, &value);
On 2014/08/13 20:09:11, nyquist wrote:
> could you guard this with url.is_valid()? Just bail out and return the same as
> not found in that case.

Done.

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
File components/dom_distiller/core/url_utils.h (right):

https://codereview.chromium.org/442503002/diff/80001/components/dom_distiller...
components/dom_distiller/core/url_utils.h:24: // Returns the value of the query
parameter for a given URL.
Did not do this word for word because that didn't make sense I think....se next
patch
On 2014/08/13 20:09:11, nyquist wrote:
> Add "value for the given |key|" after "query parameter"?

[nyquist, 2014-08-13 23:03:40]

the CL description needs to be updated. Also, add BUG= line.

First line and title could be as is.

But please add an additional description. A good starting point is the method
you added to the service interface. Also maybe add something about hasentry
(which also has doc).

https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:58: // Returns true if an
article stored has the given entry id.
"Returns whether an ar..."

https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service_unittest.cc (right):

https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service_unittest.cc:482:
EXPECT_TRUE(service_->HasEntry(entry_id));
add comment for this.

https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service_unittest.cc:557: GURL
retrieved_url(service_->GetUrlForEntry(entry_id));
Add comment for this block.

[sunangel, 2014-08-13 23:31:15]

https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service.h (right):

https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service.h:58: // Returns true if an
article stored has the given entry id.
On 2014/08/13 23:03:39, nyquist wrote:
> "Returns whether an ar..."

Done.

https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service_unittest.cc (right):

https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service_unittest.cc:482:
EXPECT_TRUE(service_->HasEntry(entry_id));
On 2014/08/13 23:03:39, nyquist wrote:
> add comment for this.

Done.

https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service_unittest.cc:557: GURL
retrieved_url(service_->GetUrlForEntry(entry_id));
On 2014/08/13 23:03:39, nyquist wrote:
> Add comment for this block.

Done.

[robliao, 2014-08-13 23:34:28]

On 2014/08/13 23:31:15, sunangel wrote:
>
https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
> File components/dom_distiller/core/dom_distiller_service.h (right):
> 
>
https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
> components/dom_distiller/core/dom_distiller_service.h:58: // Returns true if
an
> article stored has the given entry id.
> On 2014/08/13 23:03:39, nyquist wrote:
> > "Returns whether an ar..."
> 
> Done.
> 
>
https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
> File components/dom_distiller/core/dom_distiller_service_unittest.cc (right):
> 
>
https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
> components/dom_distiller/core/dom_distiller_service_unittest.cc:482:
> EXPECT_TRUE(service_->HasEntry(entry_id));
> On 2014/08/13 23:03:39, nyquist wrote:
> > add comment for this.
> 
> Done.
> 
>
https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
> components/dom_distiller/core/dom_distiller_service_unittest.cc:557: GURL
> retrieved_url(service_->GetUrlForEntry(entry_id));
> On 2014/08/13 23:03:39, nyquist wrote:
> > Add comment for this block.
> 
> Done.

It's counter intuitive, but the Subject of this bug will actually not make the
CL description.

This should be...
Subject: Retrieve article original URL from entryID
Description: Retrieve article original URL from entryID
             ...

[sunangel, 2014-08-13 23:42:15]

On 2014/08/13 23:34:28, robliao wrote:
> On 2014/08/13 23:31:15, sunangel wrote:
> >
>
https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
> > File components/dom_distiller/core/dom_distiller_service.h (right):
> > 
> >
>
https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
> > components/dom_distiller/core/dom_distiller_service.h:58: // Returns true if
> an
> > article stored has the given entry id.
> > On 2014/08/13 23:03:39, nyquist wrote:
> > > "Returns whether an ar..."
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
> > File components/dom_distiller/core/dom_distiller_service_unittest.cc
(right):
> > 
> >
>
https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
> > components/dom_distiller/core/dom_distiller_service_unittest.cc:482:
> > EXPECT_TRUE(service_->HasEntry(entry_id));
> > On 2014/08/13 23:03:39, nyquist wrote:
> > > add comment for this.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/442503002/diff/180001/components/dom_distille...
> > components/dom_distiller/core/dom_distiller_service_unittest.cc:557: GURL
> > retrieved_url(service_->GetUrlForEntry(entry_id));
> > On 2014/08/13 23:03:39, nyquist wrote:
> > > Add comment for this block.
> > 
> > Done.
> 
> It's counter intuitive, but the Subject of this bug will actually not make the
> CL description.
> 
> This should be...
> Subject: Retrieve article original URL from entryID
> Description: Retrieve article original URL from entryID
>              ...

Fixed.

[robliao, 2014-08-13 23:43:15]

https://codereview.chromium.org/442503002/diff/200001/components/dom_distille...
File
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java
(right):

https://codereview.chromium.org/442503002/diff/200001/components/dom_distille...
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:43:
private native boolean nativeHasEntry(long nativeDomDistillerServiceAndroid,
String entryId);
Linebreak above here.

https://codereview.chromium.org/442503002/diff/200001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service_android.cc (right):

https://codereview.chromium.org/442503002/diff/200001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service_android.cc:36: std::string
entry_id =
const std::string

https://codereview.chromium.org/442503002/diff/200001/components/dom_distille...
File components/dom_distiller/core/url_utils.cc (right):

https://codereview.chromium.org/442503002/diff/200001/components/dom_distille...
components/dom_distiller/core/url_utils.cc:37: std::string value;
Move declaration to after return "" (Minimal scope)

[sunangel, 2014-08-13 23:47:16]

https://codereview.chromium.org/442503002/diff/200001/components/dom_distille...
File
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java
(right):

https://codereview.chromium.org/442503002/diff/200001/components/dom_distille...
components/dom_distiller/android/java/src/org/chromium/components/dom_distiller/core/DomDistillerService.java:43:
private native boolean nativeHasEntry(long nativeDomDistillerServiceAndroid,
String entryId);
On 2014/08/13 23:43:15, robliao wrote:
> Linebreak above here.

Done.

https://codereview.chromium.org/442503002/diff/200001/components/dom_distille...
File components/dom_distiller/core/dom_distiller_service_android.cc (right):

https://codereview.chromium.org/442503002/diff/200001/components/dom_distille...
components/dom_distiller/core/dom_distiller_service_android.cc:36: std::string
entry_id =
On 2014/08/13 23:43:15, robliao wrote:
> const std::string

Done.

https://codereview.chromium.org/442503002/diff/200001/components/dom_distille...
File components/dom_distiller/core/url_utils.cc (right):

https://codereview.chromium.org/442503002/diff/200001/components/dom_distille...
components/dom_distiller/core/url_utils.cc:37: std::string value;
On 2014/08/13 23:43:15, robliao wrote:
> Move declaration to after return "" (Minimal scope)

Done.

[nyquist, 2014-08-14 00:35:12]

lgtm, but please wait for robert too

[robliao, 2014-08-14 00:52:50]

On 2014/08/14 00:35:12, nyquist wrote:
> lgtm, but please wait for robert too

lgtm with one nit.

[robliao, 2014-08-14 00:53:02]

https://codereview.chromium.org/442503002/diff/220001/components/dom_distille...
File components/dom_distiller/core/url_utils_android.cc (right):

https://codereview.chromium.org/442503002/diff/220001/components/dom_distille...
components/dom_distiller/core/url_utils_android.cc:74: .Release();
I would format this as follows:
  return base::android::
      ConvertUTF8ToJavaString(
          env, dom_distiller::url_utils::GetValueForKeyInUrl(url, key))
      .Release();

[sunangel, 2014-08-14 17:40:06]

https://codereview.chromium.org/442503002/diff/220001/components/dom_distille...
File components/dom_distiller/core/url_utils_android.cc (right):

https://codereview.chromium.org/442503002/diff/220001/components/dom_distille...
components/dom_distiller/core/url_utils_android.cc:74: .Release();
On 2014/08/14 00:53:02, robliao wrote:
> I would format this as follows:
>   return base::android::
>       ConvertUTF8ToJavaString(
>           env, dom_distiller::url_utils::GetValueForKeyInUrl(url, key))
>       .Release();

Done.

[sunangel, 2014-08-14 17:50:44]

The CQ bit was checked by sunangel@chromium.org

[commit-bot: I haz the power, 2014-08-14 17:52:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sunangel@chromium.org/442503002/300001

[sunangel, 2014-08-14 21:35:13]

The CQ bit was checked by sunangel@chromium.org

[commit-bot: I haz the power, 2014-08-14 21:38:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sunangel@chromium.org/442503002/320001

[commit-bot: I haz the power, 2014-08-15 03:17:40]

Committed patchset #7 (320001) as 289761