Closed Bug 791506 Opened 11 years ago Closed 10 years ago

[Wifi] Enable BSSID for WPS PIN

Categories

(Firefox OS Graveyard :: General, enhancement)

Product:
Firefox OS Graveyard
Component:
General
Platform:
All
Gonk (Firefox OS)
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: masashi.honma, Assigned: masashi.honma)

References

Details

Attachments

(2 files, 2 obsolete files)

Gaia patch
233 bytes, text/html
kaze
: review+
Details
Gecko Patch
7.84 KB, patch
vchang
: review+
Details | Diff | Splinter Review 
Currently BSSID for WPS PIN is fixed to 'any'.

In the ordinal environment, it's no problem.
But if there are some WPS PIN available APs, our phone try to connect them until correct PIN found. It could waste some times to connect.

This patch enables BSSID to specify appropriate AP.

This patch should be landed after Bug 790231.
Attached patch Patch file for this bug (obsolete) — — Splinter Review 

    
    

    
  
Should I update UUID of nsIWifi.idl even if I only update it's comment ?

    
    

    
  
This is the Gecko side patch of the Gaia side Pull Request #4771.

    
    

    
  
Is there any comment on this ?

If you could not understand why this patch is needed, see pull request #5066 for Gaia.
Status: UNCONFIRMED → NEW
Ever confirmed: true

    
    

    
  
(In reply to Masashi Honma from comment #2)
> Should I update UUID of nsIWifi.idl even if I only update it's comment ?

Nope no need. The uuid only has to change if the vtable of the interface changes (or a function signature changes).

    
    

    
  
Comment on attachment 661544 [details] [diff] [review]
Patch file for this bug

Do we want to default the bssid to any if the frontend doesn't pass one through?

I'm assuming that I'm only reviewing the parts of this patch that aren't covered by bug 790231 (see my comment there as well).

I'm sorry for the delay in seeing this! Please remember to CC me on bugs you file on wifi stuff!

        Attachment #661544 -
        Flags: review+

    
    

    
  
> Do we want to default the bssid to any if the frontend doesn't pass one through?

Yes.

> I'm sorry for the delay in seeing this!
> Please remember to CC me on bugs you file on wifi stuff!

OK!
Severity: normal → enhancement

    
    

    
  
(In reply to Masashi Honma from comment #7)
> > Do we want to default the bssid to any if the frontend doesn't pass one through?
> 
> Yes.

I don't see that in your patch, though?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch file after review (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
> I don't see that in your patch, though?

I attached new patch includes that.
And this patch includes patch for Bug 790231.

    
    

    
  
The Gaia side patch is PR #5528.

    
    

    
  
Comment on attachment 666280 [details] [diff] [review]
Patch file after review

Thanks! I'm curious though -- do you think it might make more sense to pass WPS via another property on ScanResult objects? That way consumers can treat "capabilities" as "security capabilities" and won't have to filter out WPS? Looking at your gaia pull request, I'm not sure that the WPS-only case is handled correctly. Let me know what you think.

        Attachment #666280 -
        Flags: review+

    
    

    
  
Masashi, please read https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Getting_reviews for the next patch you attach here. It's hard to keep track of all of the bugs & patches, so not following the procedures might mean that your patches wait longer than necessary before getting committed :( It's a pain to get used to, but it makes life a lot easier for me and other reviewers.
Assignee: nobody → masashi.honma

    
    

    
  
> That way consumers can treat "capabilities" as "security capabilities" and won't have to filter out WPS?

Yes. Current Gaia code use "capabilities" as "encryptions" property. So I will separate current "capabilities" property to "encryptions" property and use "capabilities" property as "WPS" flag and other flags container.

> I'm not sure that the WPS-only case is handled correctly.

You are right. When opposite AP is open encryption and WPS available, the Gaia UI will show "securedBy WPS".

In practice, there is not so much such AP (open and WPS available). So I will separate the property after this patch landed. Is this approach OK ?

> please read https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Getting_reviews

Thank you for good information !

    
    

    
  
(In reply to Masashi Honma from comment #14)
> In practice, there is not so much such AP (open and WPS available). So I
> will separate the property after this patch landed. Is this approach OK ?

I actually saw one the other day! Sure, that's fine. I'll check this patch in today in that case. You can attach the new patch to bug 790231.

    
    

    
  
> You can attach the new patch to bug 790231.

I attached new patch on bug 790231.

    
    

    
  
> I'll check this patch in today in that case.

I have built an "open and WPS available AP" and test the patch (Attachment #666280 [details] [diff]) with the AP. There was only display issue. There was no connectivity issue. I could connect to the AP and browse internet.

        Attachment #661544 -
        Attachment is obsolete: true

    
    

    
  
On second thought. I'd rather check this in at the same time as bug 790231 to avoid introducing a regression in order to fix it later.

    
    

    
  
Hello Masashi-san,

I'm very sorry, but I had to back out this patch :(.  The gaia pull request was not merged in time, and without this patch breaks wifi.

We need to work with kaze to get the pull request merged.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c924b386e2ea

    
    

    
  
OK. I will re-write the patch for master branch, and re-send it in few hours.

    
    

    
  
I re-wrote. See new PR #5697.

    
    

    
  
> We need to work with kaze to get the pull request merged.

Was this patch merged ?
If no, I can take a time to write new patch.
If I wrote new patch, who can review and merge it ?

    
    

    
  
> If I wrote new patch, who can review and merge it ?
Vicent, can you please help this review?
Flags: needinfo?(vchang)

    
    

    
  
(In reply to Ken Chang from comment #24)
> > If I wrote new patch, who can review and merge it ?
> Vicent, can you please help this review?
Sure, I can help to review the new patch.
Flags: needinfo?(vchang)

    
    

    
  
Vincent Chang, OK, I will re-write my patch after current work.

I have a question about Gaia/Gecko tree.
I'm building Gaia/Gecko tree by below commands.

$ git clone git://github.com/mozilla-b2g/B2G.git
$ cd B2G/
$ ./config.sh nexus-s
$ git pull
$ ./repo sync
$ ./build.sh -j8

But Gaia tree which is build by these commans is older than git master.
I can replace the older gaia tree by hand.
But I think mismatch between Gaia and Gecko interface causes some troubles.
Is there any way to get newest Gaia and appropriate Gecko tree ?

    
    

    
  
> I have a question about Gaia/Gecko tree.

I will ask this to ML.

    
    

    
  

      
      
      
      

        Attached file
          
        Gaia patch
        — 
      

    


  

    
  

        Attachment #666280 -
        Attachment is obsolete: true

    
  

        Attachment #766235 -
        Attachment mime type: text/plain → text/html

        Attachment #766235 -
        Flags: review?(vchang)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Gecko Patch
        — 
          — Splinter Review
      

    


  

        Attachment #766236 -
        Flags: review?(vchang)

    
    

    
  
I have attached two patches.

The Gaia side pull request has backward compatibility.
In other words, the Gaia code works on old and new Gecko code.
So, if you land Gaia first, you can land Gecko sefely.
After both patches are landed, I will remove the backward compatibility code from Gaia.

    
    

    
  
Vincent Chang,
Is there any comment ?

    
    

    
  
Sorry for my late response, I was in a business trip to China last week.
I'll make the review done within this week. 
For the gaia part, kaze should be the right person to review it.

    
  

        Attachment #766235 -
        Flags: review?(vchang) → review?(kaze)

    
    

    
  
Hello Masashi, I’ve just closed bug 790231 because I assumed it was obsolete with your latest patches; is that OK?

    
    

    
  
The code looks good to me for the Gaia part, but I have two (possibly silly) semantic questions.

1. what you mean by “opposite” in these two messages?

>  wpsPinInput=Input {{pin}} to an opposite device
>  wpsPinAps=Select an opposite device

Does it mean “access point”, or is there a difference that I missed? I don’t want to block on UX but I’m confused by these messages…

2. why did you change the property names? (= “security” vs “capabilities” on the Gecko side, “Security” vs “Encryptions” on the Gaia side) Is that purely semantic, or is there a technical reason to rename them?

I have the impression that forward compatibility would require less code if we kept the same property names, but I might have missed the point.

    
    

    
  
Fabien Cazenave,
Thanks for your review.

> I’ve just closed bug 790231

You can close it after this bug was fixed.
I will also close other related bugs and PRs after fix.

> 1. what you mean by “opposite” in these two messages?

The "opposite device" means "access point" right now.
But the "opposite device" could be "Group Owner" on the Wi-Fi Direct enabled device.
Currently B2G does not have Wi-Fi Direct functionality.
But we can use the word "opposite device" after B2G will implement the functionality.

> 2. why did you change the property names?

My purpose is to get "WPS" flag from scan result.
My first idea is putting "WPS" flag to existing "capabilities" property.
But Blake Kaplan proposed that "WPS" flag could be passed by another property.
(See https://bugzilla.mozilla.org/show_bug.cgi?id=791506#c12)
So I agreed and created a new property.

The new property name "security" was proposed by Blake Kaplan
(See https://bugzilla.mozilla.org/show_bug.cgi?id=790231#c9)
and agreed by you.
(See https://bugzilla.mozilla.org/show_bug.cgi?id=790231#c10)
So I'm using it.

    
    

    
  
Comment on attachment 766236 [details] [diff] [review]
Gecko Patch

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

I am fine with the patch. 

But We need to be more careful before landing the patch 
because it changes the capabilities of network object.
There might be many gaia branches, not sure if we need to 
compatible with them as well.

        Attachment #766236 -
        Flags: review?(vchang) → review+

    
    

    
  
Comment on attachment 766235 [details]
Gaia patch

(In reply to Masashi Honma from comment #36)
> The "opposite device" means "access point" right now.
> But the "opposite device" could be "Group Owner" on the Wi-Fi Direct enabled
> device.

Hmm, it still confuses me but I’ll pass the ball to the UX team and see if they want to open a follow-up.

> The new property name "security" was proposed by Blake Kaplan
> and agreed by you.

I completely forgot about that. Thanks for the clarification!

        Attachment #766235 -
        Flags: review?(kaze) → review+

    
    

    
  
Masashi, would you please squash your commits so we can merge your patch on Gaia?

    
    

    
  
Fabien Cazenave,

> Masashi, would you please squash your commits so we can merge your patch on Gaia?

Sorry for my lackness of github knowledge.

I have heared that "git push -f" command squashes commit.
But it resulted in current state.

How can I squash my patch ?

    
    

    
  
I’ll suppose “upstream” refers to mozilla-b2g/gaia, and “origin” to masap/gaia. Here’s a way to squash your commits:

> git fetch upstream
> git checkout 20130622
> git merge upstream/master
> git reset --soft upstream/master
> git commit
> git push origin 20130622 -f

Instead of doing a merge & soft reset, another way would be to rebase your patch on upstream/master.

    
    

    
  
Fabien Cazenave,
Thanks. I squashed the patch.

    
    

    
  
The Gaia part is merged on master:
https://github.com/mozilla-b2g/gaia/commit/fc469532b36759037881de2bd4ecc9a23ec12756

Now the Gecko part can be applied safely.

    
    

    
  
Vincent Chang,

Could you land Gecko part ?
Because I don't have land authority.

    
    

    
  
Here you are. 
https://hg.mozilla.org/projects/birch/rev/43d14cca5953

You can also enter the keyword "checkin-needed" next time. :-)

    
    

    
  
> You can also enter the keyword "checkin-needed" next time. :-)

Oh, thanks.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/43d14cca5953
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.