Changes for crawling:

[kpr...@google.com]

Reviewers: amitmanjhi,

Description:
Changes for crawling:
- CrawlableHyperlink
- client-side changes to Showcase sample

Please review this at http://gwt-code-reviews.appspot.com/161801

Affected files:
M samples/showcase/src/com/google/gwt/sample/showcase/client/Showcase.java
M samples/showcase/war/Showcase.html
A user/src/com/google/gwt/user/client/ui/CrawlableHyperlink.java
A user/test/com/google/gwt/user/client/ui/CrawlableHyperlinkTest.java

[amitm...@google.com]

http://gwt-code-reviews.appspot.com/161801/diff/1/3
File samples/showcase/war/Showcase.html (right):

http://gwt-code-reviews.appspot.com/161801/diff/1/3#newcode4
Line 4: <script language='javascript'>
Any disadvantage to leaving the title here (even though you are setting
it later)? At least, browsers that can't run JS can see the title.

http://gwt-code-reviews.appspot.com/161801/diff/1/4
File user/src/com/google/gwt/user/client/ui/CrawlableHyperlink.java
(right):

http://gwt-code-reviews.appspot.com/161801/diff/1/4#newcode58
Line 58: }
Why override these constructors if the override is not doing anything
useful?

Shouldn't over-riding just the setTargetHistoryToken method and having a
more descriptive Javadoc for this class suffice?

http://gwt-code-reviews.appspot.com/161801/diff/1/5
File user/test/com/google/gwt/user/client/ui/CrawlableHyperlinkTest.java
(right):

http://gwt-code-reviews.appspot.com/161801/diff/1/5#newcode28
Line 28: }
change to "com.google.gwt.user.DebugTest", as in HyperLinkTest.

http://gwt-code-reviews.appspot.com/161801/diff/1/5#newcode36
Line 36:
Arguments of assertEquals must be swapped. It is assertEquals(expected,
actual).

http://gwt-code-reviews.appspot.com/161801/diff/1/5#newcode41
Line 41: assertEquals(historyToken, "!myToken");
historyToken can be inlined everywhere.

http://gwt-code-reviews.appspot.com/161801

[kpr...@google.com]

Reviewers: amtimanjhi_google.com,

Description:
Changes for crawling:
- CrawlableHyperlink
- client-side changes to Showcase sample

Review at http://gwt-code-reviews.appspot.com/161801

Please review this at http://gwt-code-reviews.appspot.com/163802

[kpr...@google.com]

http://gwt-code-reviews.appspot.com/161801/diff/1/3
File samples/showcase/war/Showcase.html (right):

http://gwt-code-reviews.appspot.com/161801/diff/1/3#newcode4
Line 4: <script language='javascript'>

The reason I add the title programmatically is because it changes to
reflect the widget that is being shown off. This makes for more
descriptive titles, which is especially important for indexing.

[As an aside, I personally feel there should be nothing or close to
nothing in the html file. I see your point, but if your browser doesn't
run JS, this title isn't going to help you a whole lot, either... But if
you feel strongly about it, I'll put it back in, but it will just be
overridden if the browser does run JS.]

On 2010/03/08 23:29:44, amitmanjhi wrote:
> Any disadvantage to leaving the title here (even though you are
setting it
> later)? At least, browsers that can't run JS can see the title.

http://gwt-code-reviews.appspot.com/161801/diff/1/4
File user/src/com/google/gwt/user/client/ui/CrawlableHyperlink.java
(right):

http://gwt-code-reviews.appspot.com/161801/diff/1/4#newcode58
Line 58: }
I'm not sure I understand. Because constructors aren't inherited, I
wouldn't be able to do Hyperlink link = new CrawlableHyperlink("Click
Me", "myToken"); if I didn't override them. Do you have a specific
suggestion?

Great point about the more descriptive javadoc. I hope I've improved
it.

On 2010/03/08 23:29:44, amitmanjhi wrote:
> Why override these constructors if the override is not doing anything
useful?

> Shouldn't over-riding just the setTargetHistoryToken method and having
a more
> descriptive Javadoc for this class suffice?

http://gwt-code-reviews.appspot.com/161801/diff/1/5
File user/test/com/google/gwt/user/client/ui/CrawlableHyperlinkTest.java
(right):

http://gwt-code-reviews.appspot.com/161801/diff/1/5#newcode28
Line 28: }

On 2010/03/08 23:29:44, amitmanjhi wrote:
> change to "com.google.gwt.user.DebugTest", as in HyperLinkTest.

Done.

http://gwt-code-reviews.appspot.com/161801/diff/1/5#newcode36
Line 36:
Thanks!

Done.

On 2010/03/08 23:29:44, amitmanjhi wrote:
> Arguments of assertEquals must be swapped. It is
assertEquals(expected, actual).

http://gwt-code-reviews.appspot.com/161801/diff/1/5#newcode41
Line 41: assertEquals(historyToken, "!myToken");

On 2010/03/08 23:29:44, amitmanjhi wrote:
> historyToken can be inlined everywhere.

Done.

http://gwt-code-reviews.appspot.com/161801

[kpr...@google.com]

Reviewers: amitmanjhi,

Description:
Changes for crawling:
- CrawlableHyperlink
- client-side changes to Showcase sample

Review at http://gwt-code-reviews.appspot.com/161801

Please review this at http://gwt-code-reviews.appspot.com/165801

[amitm...@google.com]

On 2010/03/09 18:27:31, kathrin wrote:

Continuing the code review here. The only remaining issue I have is with
the 3 constructors in CrawlableHyperlink that don't do anything. Do you
need to override all 3? Also, while you are at it, perhaps you can
deprecate/refactor/cleanup some of the constructor code in Hyperlink.
Thanks!

http://gwt-code-reviews.appspot.com/165801

[kpr...@google.com]

http://gwt-code-reviews.appspot.com/161801

[kpr...@google.com]

On 2010/03/11 16:45:34, kathrin wrote:

Hi Amit,

after a lengthy discussion with Joel, we decided to get rid of the
CrawlableHyperlink widget. The issue is that it doesn't add enough
useful functionality, because the app writer still needs to handle the
"!" when actually "navigating" the app to a history state. For this
reason, we will recommend that people do this process manually, which is
the same amount of work.

I got rid of the widget and made the necessary changes to Showcase -
could you have another look?

Thanks!
kathrin

http://gwt-code-reviews.appspot.com/161801

[amitm...@google.com]

LGTM -- I assume you have checked that it still works correctly. The
only minor comment I have is extracting out the string constant "!" used
at two places.

http://gwt-code-reviews.appspot.com/161801

[John Tamplin]

On Thu, Mar 11, 2010 at 11:48 AM, <kpr...@google.com> wrote:

after a lengthy discussion with Joel, we decided to get rid of the
CrawlableHyperlink widget.  The issue is that it doesn't add enough
useful functionality, because the app writer still needs to handle the
"!" when actually "navigating" the app to a history state.  For this
reason, we will recommend that people do this process manually, which is
the same amount of work.

I think eventually we want to have more structured history support built on top of the basic solution to getting/storing the hash fragment, such as support for tag/values and/or hierarchical history values.  At that point, we should consider crawlable history states and revisit making Hyperlink smarter about supporting them (either directly or via CrawlableHyperlink).

 

--
John A. Tamplin
Software Engineer (GWT), Google