Re: Review Request 34673: GadgetSite setModuleId_ only sets the module id if rendering the first time

2015-05-27 Thread Ryan Baxter 


> On May 27, 2015, 7:43 p.m., Doug Davies wrote:
> > Added another patch that tries to re-use the previous moduleId off the 
> > cached token.  It also checks if mid is defined (as suggested by Ryan).

Looks good Doug, is there any kind of unit tests we can add around this?


- Ryan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34673/#review85425
---


On May 27, 2015, 7:39 p.m., Doug Davies wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34673/
> ---
> 
> (Updated May 27, 2015, 7:39 p.m.)
> 
> 
> Review request for shindig.
> 
> 
> Bugs: SHINDIG-1995
> https://issues.apache.org/jira/browse/SHINDIG-1995
> 
> 
> Repository: shindig
> 
> 
> Description
> ---
> 
> GadgetSite setModuleId_ only sets the module id if rendering the first time
> 
> 
> Diffs
> -
> 
>   
> trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js
>  1681771 
> 
> Diff: https://reviews.apache.org/r/34673/diff/
> 
> 
> Testing
> ---
> 
> See bug (SHINDIG-1995) for details on how to test this.
> 
> 
> Thanks,
> 
> Doug Davies
> 
>

Re: Review Request 34673: GadgetSite setModuleId_ only sets the module id if rendering the first time

2015-05-27 Thread Doug Davies 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34673/
---

(Updated May 27, 2015, 7:39 p.m.)


Review request for shindig.


Bugs: SHINDIG-1995
https://issues.apache.org/jira/browse/SHINDIG-1995


Repository: shindig


Description
---

GadgetSite setModuleId_ only sets the module id if rendering the first time


Diffs (updated)
-

  
trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js
 1681771 

Diff: https://reviews.apache.org/r/34673/diff/


Testing
---

See bug (SHINDIG-1995) for details on how to test this.


Thanks,

Doug Davies

Re: Review Request 34673: GadgetSite setModuleId_ only sets the module id if rendering the first time

2015-05-27 Thread Doug Davies 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34673/#review85425
---


Added another patch that tries to re-use the previous moduleId off the cached 
token.  It also checks if mid is defined (as suggested by Ryan).

- Doug Davies


On May 27, 2015, 7:39 p.m., Doug Davies wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34673/
> ---
> 
> (Updated May 27, 2015, 7:39 p.m.)
> 
> 
> Review request for shindig.
> 
> 
> Bugs: SHINDIG-1995
> https://issues.apache.org/jira/browse/SHINDIG-1995
> 
> 
> Repository: shindig
> 
> 
> Description
> ---
> 
> GadgetSite setModuleId_ only sets the module id if rendering the first time
> 
> 
> Diffs
> -
> 
>   
> trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js
>  1681771 
> 
> Diff: https://reviews.apache.org/r/34673/diff/
> 
> 
> Testing
> ---
> 
> See bug (SHINDIG-1995) for details on how to test this.
> 
> 
> Thanks,
> 
> Doug Davies
> 
>

Re: Review Request 34673: GadgetSite setModuleId_ only sets the module id if rendering the first time

2015-05-27 Thread Doug Davies 


> On May 27, 2015, 4:21 a.m., Ryan Baxter wrote:
> > trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js,
> >  line 128
> > 
> >
> > Should this be surrounded in the same if statement as above?
> 
> Doug Davies wrote:
> No, that's the problem.  "!self.service_.getCachedGadgetToken(url)" is 
> FALSE (token exists because we are re-rendering).  In that case we drop 
> through, but the moduleId is never set.
> 
> Ryan Baxter wrote:
> was actually talking about this if statement
> 
> ```
> if (mid || mid == 0) {
>   self.moduleId_ = mid;
> }
> ```

Possibly.  I was figuring the outer if statement (mid && this.moduleId_ != mid) 
was taking care of this. But I can check again if desired.


- Doug


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34673/#review85315
---


On May 26, 2015, 6:26 p.m., Doug Davies wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34673/
> ---
> 
> (Updated May 26, 2015, 6:26 p.m.)
> 
> 
> Review request for shindig.
> 
> 
> Bugs: SHINDIG-1995
> https://issues.apache.org/jira/browse/SHINDIG-1995
> 
> 
> Repository: shindig
> 
> 
> Description
> ---
> 
> GadgetSite setModuleId_ only sets the module id if rendering the first time
> 
> 
> Diffs
> -
> 
>   
> trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js
>  1681771 
> 
> Diff: https://reviews.apache.org/r/34673/diff/
> 
> 
> Testing
> ---
> 
> See bug (SHINDIG-1995) for details on how to test this.
> 
> 
> Thanks,
> 
> Doug Davies
> 
>

Re: Review Request 34673: GadgetSite setModuleId_ only sets the module id if rendering the first time

2015-05-27 Thread Ryan Baxter 


> On May 27, 2015, 4:21 a.m., Ryan Baxter wrote:
> > trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js,
> >  line 128
> > 
> >
> > Should this be surrounded in the same if statement as above?
> 
> Doug Davies wrote:
> No, that's the problem.  "!self.service_.getCachedGadgetToken(url)" is 
> FALSE (token exists because we are re-rendering).  In that case we drop 
> through, but the moduleId is never set.

was actually talking about this if statement

```
if (mid || mid == 0) {
  self.moduleId_ = mid;
}
```


- Ryan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34673/#review85315
---


On May 26, 2015, 6:26 p.m., Doug Davies wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34673/
> ---
> 
> (Updated May 26, 2015, 6:26 p.m.)
> 
> 
> Review request for shindig.
> 
> 
> Bugs: SHINDIG-1995
> https://issues.apache.org/jira/browse/SHINDIG-1995
> 
> 
> Repository: shindig
> 
> 
> Description
> ---
> 
> GadgetSite setModuleId_ only sets the module id if rendering the first time
> 
> 
> Diffs
> -
> 
>   
> trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js
>  1681771 
> 
> Diff: https://reviews.apache.org/r/34673/diff/
> 
> 
> Testing
> ---
> 
> See bug (SHINDIG-1995) for details on how to test this.
> 
> 
> Thanks,
> 
> Doug Davies
> 
>

Re: Review Request 34673: GadgetSite setModuleId_ only sets the module id if rendering the first time

2015-05-27 Thread Doug Davies 


> On May 27, 2015, 4:21 a.m., Ryan Baxter wrote:
> > trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js,
> >  line 128
> > 
> >
> > Should this be surrounded in the same if statement as above?

No, that's the problem.  "!self.service_.getCachedGadgetToken(url)" is FALSE 
(token exists because we are re-rendering).  In that case we drop through, but 
the moduleId is never set.


- Doug


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34673/#review85315
---


On May 26, 2015, 6:26 p.m., Doug Davies wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34673/
> ---
> 
> (Updated May 26, 2015, 6:26 p.m.)
> 
> 
> Review request for shindig.
> 
> 
> Bugs: SHINDIG-1995
> https://issues.apache.org/jira/browse/SHINDIG-1995
> 
> 
> Repository: shindig
> 
> 
> Description
> ---
> 
> GadgetSite setModuleId_ only sets the module id if rendering the first time
> 
> 
> Diffs
> -
> 
>   
> trunk/features/src/main/javascript/features/container.site.gadget/gadget_site.js
>  1681771 
> 
> Diff: https://reviews.apache.org/r/34673/diff/
> 
> 
> Testing
> ---
> 
> See bug (SHINDIG-1995) for details on how to test this.
> 
> 
> Thanks,
> 
> Doug Davies
> 
>