[Offline Pages] Fixing bookmarking offline page will save file path.

[Offline Pages] Fixing bookmarking offline page will save file path.

Fixing the issue where in offline mode, bookmarking an offline page
would save the file path instead of the original path.

BUG=623243
Committed: https://crrev.com/e09dff921a487d398749812d35804267d883aa55
Cr-Commit-Position: refs/heads/master@{#407520}

[romax, 2016-07-14 23:54:57]

The CQ bit was checked by romax@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 23:55:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dmitry Titov, 2016-07-15 00:07:42]

dimich@chromium.org changed reviewers:
+ dimich@chromium.org, dimich@google.com

[Dmitry Titov, 2016-07-15 00:07:43]

https://codereview.chromium.org/2144423002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://codereview.chromium.org/2144423002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:75:
String url = DomDistillerUrlUtils.getOriginalUrlFromDistillerUrl(tabUrl);
I think tab.getOfflinePageOriginalUrl() implementation is already trying to pull
the distiller URL... Maybe if it isOfflinePage we don't need to do it? If
distiller is involved, what page are we ending up snapshotting?

[commit-bot: I haz the power, 2016-07-15 00:35:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-15 00:35:06]

Dry run: This issue passed the CQ dry run.

[romax, 2016-07-15 01:45:53]

Description was changed from

==========
[Offline Pages] Fixing bookmarking offline page will save file path.

Fixing the issue where in offline mode, bookmarking an offline page
would save the file path instead of the original path.

BUG=623243
==========

to

==========
[Offline Pages] Fixing bookmarking offline page will save file path.

Fixing the issue where in offline mode, bookmarking an offline page
would save the file path instead of the original path.

BUG=623243
==========

[romax, 2016-07-15 01:45:53]

romax@chromium.org changed reviewers:
+ ianwen@chromium.org
- dimich@google.com

[romax, 2016-07-15 01:46:10]

PTAL, thanks!

https://codereview.chromium.org/2144423002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://codereview.chromium.org/2144423002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:75:
String url = DomDistillerUrlUtils.getOriginalUrlFromDistillerUrl(tabUrl);
On 2016/07/15 00:07:43, Dmitry Titov wrote:
> I think tab.getOfflinePageOriginalUrl() implementation is already trying to
pull
> the distiller URL... Maybe if it isOfflinePage we don't need to do it? If
> distiller is involved, what page are we ending up snapshotting?

yeah using an if instead.
And I don't really understand why it would affect snapshotting, this is when
adding  bookmarks, while saving pages it's in ChromeActivity.java and it would
use the tab.

[Ian Wen, 2016-07-15 17:33:20]

Thanks for fixing the bug!

https://chromiumcodereview.appspot.com/2144423002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://chromiumcodereview.appspot.com/2144423002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:76:
url = tab.getOfflinePageOriginalUrl();
I assume getOfflinePageOriginalUrl() will be called in many places like this.
Shall we follow the pattern of DomDistiller and create a simple static method,
instead of if-elses?

I see OfflinePageUtils::MaybeGetOnlineURLForOfflineURL() is already in native.
Shall we expose it to java and use that instead?

:)

[Dmitry Titov, 2016-07-15 18:02:38]

Is it possible to have a test for this?

https://chromiumcodereview.appspot.com/2144423002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://chromiumcodereview.appspot.com/2144423002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:76:
url = tab.getOfflinePageOriginalUrl();
On 2016/07/15 17:33:20, Ian Wen wrote:
> I assume getOfflinePageOriginalUrl() will be called in many places like this.
> Shall we follow the pattern of DomDistiller and create a simple static method,
> instead of if-elses?
> 
> I see OfflinePageUtils::MaybeGetOnlineURLForOfflineURL() is already in native.
> Shall we expose it to java and use that instead?
> 
> :)

Let me try to answer this for romax@... There is no direct symmetry here, here
is more context:

- the distiller URL contains original URL, so it's just a GURL unpack operation,
not so with offline pages (they have file:// URLs of local resource and a DB
doing mapping)
- the Maybe* methods are deprecated, we are looking to remove them. They are
"maybe" because they are sync and may simply not work if DB was not yet
asynchronously loaded. We should not use those.
- it's good to go through tab since we are converting most of the
mapping/resolving methods to be async and those consumers that can not be async
will use tab-cached info.

So it seems that having the check and non-static method is ok, at least for now.
This pattern is already used in a couple places, we'd rather replace it with
async one soon instead of adding another one now...

[Ian Wen, 2016-07-15 18:38:23]

ianwen@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[Ian Wen, 2016-07-15 18:38:24]

https://chromiumcodereview.appspot.com/2144423002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://chromiumcodereview.appspot.com/2144423002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:76:
url = tab.getOfflinePageOriginalUrl();
On 2016/07/15 18:02:38, Dmitry Titov wrote:
> On 2016/07/15 17:33:20, Ian Wen wrote:
> > I assume getOfflinePageOriginalUrl() will be called in many places like
this.
> > Shall we follow the pattern of DomDistiller and create a simple static
method,
> > instead of if-elses?
> > 
> > I see OfflinePageUtils::MaybeGetOnlineURLForOfflineURL() is already in
native.
> > Shall we expose it to java and use that instead?
> > 
> > :)
> 
> Let me try to answer this for romax@... There is no direct symmetry here, here
> is more context:
> 
> - the distiller URL contains original URL, so it's just a GURL unpack
operation,
> not so with offline pages (they have file:// URLs of local resource and a DB
> doing mapping)
> - the Maybe* methods are deprecated, we are looking to remove them. They are
> "maybe" because they are sync and may simply not work if DB was not yet
> asynchronously loaded. We should not use those.
> - it's good to go through tab since we are converting most of the
> mapping/resolving methods to be async and those consumers that can not be
async
> will use tab-cached info.
> 
> So it seems that having the check and non-static method is ok, at least for
now.
> This pattern is already used in a couple places, we'd rather replace it with
> async one soon instead of adding another one now...

Thanks for the explanation!

Then I wonder if it makes sense to create a method in Tab called
getOriginalUrl(), which takes care of offline pages as well as dom distiller.
Then other places do not need to have lines like #74 -#79. Another way is to
simply replace getOfflinePageOriginalUrl() to getOriginalUrl() and use that in
all places.

IMO get url from a tab should be such a common case that we should strive to
make it simple and convenient.

Added dfalcantara@ to this CL and he could review the upcoming change in
Tab.java. :)

[Dmitry Titov, 2016-07-15 20:04:42]

On 2016/07/15 18:38:24, Ian Wen wrote:
>
https://chromiumcodereview.appspot.com/2144423002/diff/20001/chrome/android/j...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
> (right):
> 
>
https://chromiumcodereview.appspot.com/2144423002/diff/20001/chrome/android/j...
>
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:76:
> url = tab.getOfflinePageOriginalUrl();
> On 2016/07/15 18:02:38, Dmitry Titov wrote:
> > On 2016/07/15 17:33:20, Ian Wen wrote:
> > > I assume getOfflinePageOriginalUrl() will be called in many places like
> this.
> > > Shall we follow the pattern of DomDistiller and create a simple static
> method,
> > > instead of if-elses?
> > > 
> > > I see OfflinePageUtils::MaybeGetOnlineURLForOfflineURL() is already in
> native.
> > > Shall we expose it to java and use that instead?
> > > 
> > > :)
> > 
> > Let me try to answer this for romax@... There is no direct symmetry here,
here
> > is more context:
> > 
> > - the distiller URL contains original URL, so it's just a GURL unpack
> operation,
> > not so with offline pages (they have file:// URLs of local resource and a DB
> > doing mapping)
> > - the Maybe* methods are deprecated, we are looking to remove them. They are
> > "maybe" because they are sync and may simply not work if DB was not yet
> > asynchronously loaded. We should not use those.
> > - it's good to go through tab since we are converting most of the
> > mapping/resolving methods to be async and those consumers that can not be
> async
> > will use tab-cached info.
> > 
> > So it seems that having the check and non-static method is ok, at least for
> now.
> > This pattern is already used in a couple places, we'd rather replace it with
> > async one soon instead of adding another one now...
> 
> Thanks for the explanation!
> 
> Then I wonder if it makes sense to create a method in Tab called
> getOriginalUrl(), which takes care of offline pages as well as dom distiller.
> Then other places do not need to have lines like #74 -#79. Another way is to
> simply replace getOfflinePageOriginalUrl() to getOriginalUrl() and use that in
> all places.
> 
> IMO get url from a tab should be such a common case that we should strive to
> make it simple and convenient.
> 
> Added dfalcantara@ to this CL and he could review the upcoming change in
> Tab.java. :)

+1 for tab.getOriginalUrl() that takes care about both distiller and offline.

[romax, 2016-07-16 00:13:53]

https://chromiumcodereview.appspot.com/2144423002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://chromiumcodereview.appspot.com/2144423002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:76:
url = tab.getOfflinePageOriginalUrl();
On 2016/07/15 18:38:24, Ian Wen wrote:
> On 2016/07/15 18:02:38, Dmitry Titov wrote:
> > On 2016/07/15 17:33:20, Ian Wen wrote:
> > > I assume getOfflinePageOriginalUrl() will be called in many places like
> this.
> > > Shall we follow the pattern of DomDistiller and create a simple static
> method,
> > > instead of if-elses?
> > > 
> > > I see OfflinePageUtils::MaybeGetOnlineURLForOfflineURL() is already in
> native.
> > > Shall we expose it to java and use that instead?
> > > 
> > > :)
> > 
> > Let me try to answer this for romax@... There is no direct symmetry here,
here
> > is more context:
> > 
> > - the distiller URL contains original URL, so it's just a GURL unpack
> operation,
> > not so with offline pages (they have file:// URLs of local resource and a DB
> > doing mapping)
> > - the Maybe* methods are deprecated, we are looking to remove them. They are
> > "maybe" because they are sync and may simply not work if DB was not yet
> > asynchronously loaded. We should not use those.
> > - it's good to go through tab since we are converting most of the
> > mapping/resolving methods to be async and those consumers that can not be
> async
> > will use tab-cached info.
> > 
> > So it seems that having the check and non-static method is ok, at least for
> now.
> > This pattern is already used in a couple places, we'd rather replace it with
> > async one soon instead of adding another one now...
> 
> Thanks for the explanation!
> 
> Then I wonder if it makes sense to create a method in Tab called
> getOriginalUrl(), which takes care of offline pages as well as dom distiller.
> Then other places do not need to have lines like #74 -#79. Another way is to
> simply replace getOfflinePageOriginalUrl() to getOriginalUrl() and use that in
> all places.
> 
> IMO get url from a tab should be such a common case that we should strive to
> make it simple and convenient.
> 
> Added dfalcantara@ to this CL and he could review the upcoming change in
> Tab.java. :)

made the change to merge the paths in Tab.java. And where would be good place to
add a test as mentioned by dimich@?

[Ian Wen, 2016-07-16 01:04:54]

https://chromiumcodereview.appspot.com/2144423002/diff/40001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://chromiumcodereview.appspot.com/2144423002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2904: public
String getOriginalUrl() {
We should convert some other places that we use getOfflinePageOriginalUrl() to
useing this method instead. For example, ToolbarModelImpl.

[romax, 2016-07-18 22:38:09]

PTAL, thanks!

https://chromiumcodereview.appspot.com/2144423002/diff/40001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://chromiumcodereview.appspot.com/2144423002/diff/40001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2904: public
String getOriginalUrl() {
On 2016/07/16 01:04:54, Ian Wen wrote:
> We should convert some other places that we use getOfflinePageOriginalUrl() to
> useing this method instead. For example, ToolbarModelImpl.

Done.

[Ian Wen, 2016-07-18 23:15:42]

lgtm

[romax, 2016-07-19 20:57:58]

ping.
dimich@ and dfalcantara@, PTAL, thanks.

[gone, 2016-07-19 21:19:16]

lgtm

[gone, 2016-07-19 21:20:15]

One thing that we'll need to be careful about is that DOM Distiller's version of
getOriginalUrl requires native.  I think we're fine with the current setup, but
we'll need to watch for crashes closely.

[romax, 2016-07-19 21:23:35]

On 2016/07/19 21:20:15, dfalcantara wrote:
> One thing that we'll need to be careful about is that DOM Distiller's version
of
> getOriginalUrl requires native.  I think we're fine with the current setup,
but
> we'll need to watch for crashes closely.

Sure, and the offline page version uses native as well. I'll keep an eye on
crashes if there're any.

Thanks!

[Dmitry Titov, 2016-07-21 02:20:25]

lgtm

lgtm

[Dmitry Titov, 2016-07-21 02:20:47]

The CQ bit was checked by dimich@chromium.org

[Dmitry Titov, 2016-07-21 02:20:47]

The CQ bit was unchecked by dimich@chromium.org

[romax, 2016-07-25 17:49:12]

The CQ bit was checked by romax@chromium.org

[commit-bot: I haz the power, 2016-07-25 17:49:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 18:22:32]

Description was changed from

==========
[Offline Pages] Fixing bookmarking offline page will save file path.

Fixing the issue where in offline mode, bookmarking an offline page
would save the file path instead of the original path.

BUG=623243
==========

to

==========
[Offline Pages] Fixing bookmarking offline page will save file path.

Fixing the issue where in offline mode, bookmarking an offline page
would save the file path instead of the original path.

BUG=623243
==========

[commit-bot: I haz the power, 2016-07-25 18:22:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-25 18:24:04]

Description was changed from

==========
[Offline Pages] Fixing bookmarking offline page will save file path.

Fixing the issue where in offline mode, bookmarking an offline page
would save the file path instead of the original path.

BUG=623243
==========

to

==========
[Offline Pages] Fixing bookmarking offline page will save file path.

Fixing the issue where in offline mode, bookmarking an offline page
would save the file path instead of the original path.

BUG=623243

Committed: https://crrev.com/e09dff921a487d398749812d35804267d883aa55
Cr-Commit-Position: refs/heads/master@{#407520}
==========

[commit-bot: I haz the power, 2016-07-25 18:24:06]

Patchset 4 (id:??) landed as
https://crrev.com/e09dff921a487d398749812d35804267d883aa55
Cr-Commit-Position: refs/heads/master@{#407520}